Re: [PING^2][PATCH][final] Handle compiler-generated asm insn

[Tom de Vries <tdevries@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4278 bytes --]

On 3/21/22 14:49, Richard Biener wrote:
> On Mon, Mar 21, 2022 at 12:50 PM Tom de Vries <tdevries@suse.de> wrote:
>>
>> On 3/21/22 08:58, Richard Biener wrote:
>>> On Thu, Mar 17, 2022 at 4:10 PM Tom de Vries via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>
>>>> On 3/9/22 13:50, Tom de Vries wrote:
>>>>> On 2/22/22 14:55, Tom de Vries wrote:
>>>>>> Hi,
>>>>>>
>>>>>> For the nvptx port, with -mptx-comment we have in pr53465.s:
>>>>>> ...
>>>>>>            // #APP
>>>>>> // 9 "gcc/testsuite/gcc.c-torture/execute/pr53465.c" 1
>>>>>>            // Start: Added by -minit-regs=3:
>>>>>>            // #NO_APP
>>>>>>                    mov.u32 %r26, 0;
>>>>>>            // #APP
>>>>>> // 9 "gcc/testsuite/gcc.c-torture/execute/pr53465.c" 1
>>>>>>            // End: Added by -minit-regs=3:
>>>>>>            // #NO_APP
>>>>>> ...
>>>>>>
>>>>>> The comments where generated using the compiler-generated equivalent of:
>>>>>> ...
>>>>>>      asm ("// Comment");
>>>>>> ...
>>>>>> but both the printed location and the NO_APP/APP are unnecessary for a
>>>>>> compiler-generated asm insn.
>>>>>>
>>>>>> Fix this by handling ASM_INPUT_SOURCE_LOCATION == UNKNOWN_LOCATION in
>>>>>> final_scan_insn_1, such what we simply get:
>>>>>> ...
>>>>>>            // Start: Added by -minit-regs=3:
>>>>>>                    mov.u32 %r26, 0;
>>>>>>            // End: Added by -minit-regs=3:
>>>>>> ...
>>>>>>
>>>>>> Tested on nvptx.
>>>>>>
>>>>>> OK for trunk?
>>>>>>
>>>>>
>>>>
>>>> Ping^2.
>>>>
>>>> Tobias just reported an ICE in PR104968, and this patch fixes it.
>>>>
>>>> I'd like to known whether this patch is acceptable for stage 4 or not.
>>>>
>>>> If not, I need to fix PR104968 in a different way.  Say, disable
>>>> -mcomment by default, or trying harder to propagate source info on
>>>> outlined functions.
>>>
>>
>> Hi,
>>
>> thanks for the review.
>>
>>> Usually targets use UNSPECs to emit compiler-generated "asm"
>>> instructions.
>>
>> Ack. [ I could go down that route eventually, but for now I'm hoping to
>> implement this without having to change the port. ]
>>
>>> I think an unknown location is a reasonable but not
>>> the best way to identify 'compiler-generated', we might lose
>>> the location through optimization.  (why does it not use
>>> the INSN_LOCATION?)
>>>
>>
>> I don't know.  FWIW, at the time that ASM_INPUT_SOURCE_LOCATION was
>> introduced (2007), there was no INSN_LOCATION yet (introduced in 2012),
>> only INSN_LOCATOR, my guess is that it has something to do with that.
>>
>>> Rather than a location I'd use sth like DECL_ARTIFICIAL to
>>> disable 'user-mangling', do we have something like that for
>>> ASM or an insn in general?
>>
>> Haven't found it.
>>
>>> If not maybe there's an unused
>>> bit on ASMs we can enable this way.
>>
>> Done.  I've used the jump flag for that.
>>
>> Updated, untested patch attached.
>>
>> Is this what you meant?
> 
> Hmm.  I now read that ASM_INPUT is in every PATTERN of an insn

Maybe I misunderstand, but that sounds incorrect to me.  That is, can 
you point me to where you read that?

Maybe you're referring to the fact that an ASM_INPUT may occur inside an 
ASM_OPERANDS, as "a convenient way to hold a string" (quoting rtl.def)?

> and wonder how this all works out there.  That is, by default the
> ASM_INPUT would be artificial (for regular define_insn) but asm("")
> in source would mark them ASM_INPUT_USER_P or so.
> 

If you're suggesting to make it by default artificial, then that doesn't 
sound like a bad idea to me.  In this iteration I haven't implemented 
this (yet), but instead explicitly marked as artificial some other uses 
of ASM_INPUT.

> But then I know nothing here.  I did expect us to look at
> ASM_OPERANDS instead of just ASM_INPUT (but the code you
> are changing is about ASM_INPUT).
> 

I extended the rationale in the commit log a bit to include a 
description of what the rtl-equivalent of 'asm ("// Comment")' looks 
like, and there's no ASM_OPERANDS there.

> That said, the comments should probably explicitely say this
> is about ASM_INPUT in an ASM_OPERANDS  instruction
> template, not some other pattern.
> 

AFAIU, this isn't about an ASM_INPUT in an ASM_OPERANDS  instruction 
template, so at this point I haven't updated the comment.

Thanks,
- Tom

[-- Attachment #2: 0015-final-Handle-compiler-generated-asm-insn.patch --]
[-- Type: text/x-patch, Size: 6235 bytes --]

[final] Handle compiler-generated asm insn

For the nvptx port, with -mptx-comment we have for test-case pr53465.c at
mach:
...
(insn 66 43 65 3 (asm_input ("// Start: Added by -minit-regs=3:")) -1
     (nil))
(insn 65 66 67 3 (set (reg/v:SI 26 [ d ])
        (const_int 0 [0])) 6 {*movsi_insn}
     (nil))
(insn 67 65 44 3 (asm_input ("// End: Added by -minit-regs=3:")) -1
     (nil))
...
and in pr53465.s:
...
        // #APP
// 9 "gcc/testsuite/gcc.c-torture/execute/pr53465.c" 1
        // Start: Added by -minit-regs=3:
        // #NO_APP
                mov.u32 %r26, 0;
        // #APP
// 9 "gcc/testsuite/gcc.c-torture/execute/pr53465.c" 1
        // End: Added by -minit-regs=3:
        // #NO_APP
...

[ The comment insns were modelled after:
...
  asm ("// Comment");
...
which expands to:
...
(insn 5 2 6 2 (parallel [
            (asm_input/v ("// Comment") test.c:4)
            (clobber (mem:BLK (scratch) [0  A8]))
        ]) "test.c":4:3 -1
     (nil))
...
Note btw the differences: the comment insn has no clobber, and ASM_INPUT is
not volatile. ]

Both the printed location and the NO_APP/APP are unnecessary for a
compiler-generated asm insn.

Fix this by:
- adding new flag ASM_INPUT_ARTIFICIAL_P
- setting it in common code where that is appropriate
- in gen_comment:
  - setting ASM_INPUT_ARTIFICIAL_P to 1
  - setting ASM_INPUT_SOURCE_LOCATION to UNKNOWN_LOCATION,
- in final_scan_insn_1:
  - handling ASM_INPUT_SOURCE_LOCATION == UNKNOWN_LOCATION and
  ASM_INPUT_ARTIFICIAL_P
such what we simply get:
...
        // Start: Added by -minit-regs=3:
                mov.u32 %r26, 0;
        // End: Added by -minit-regs=3:
...

Tested on nvptx.

gcc/ChangeLog:

2022-02-21  Tom de Vries  <tdevries@suse.de>

	PR rtl-optimization/104596
	* rtl.h (struct rtx_def): Document use of jump flag in ASM_INPUT.
	(ASM_INPUT_ARTIFICIAL_P): New macro.
	* cfgexpand.cc (expand_asm_stmt): Use ASM_INPUT_ARTIFICIAL_P.
	* emit-rtl.cc (gen_blockage): Same.
	* config/nvptx/nvptx.cc (gen_comment): Use gen_rtx_ASM_INPUT instead
	of gen_rtx_ASM_INPUT_loc.  Set ASM_INPUT_ARTIFICIAL_P.
	* final.cc (final_scan_insn_1): Handle
	ASM_INPUT_SOURCE_LOCATION == UNKNOWN_LOCATION and
	ASM_INPUT_ARTIFICIAL_P.

---
 gcc/cfgexpand.cc          |  4 ++++
 gcc/config/nvptx/nvptx.cc |  5 +++--
 gcc/emit-rtl.cc           |  3 +++
 gcc/final.cc              | 18 ++++++++++++------
 gcc/rtl.h                 |  3 +++
 5 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
index d3cc77d2ca98..9fddec1b3c2d 100644
--- a/gcc/cfgexpand.cc
+++ b/gcc/cfgexpand.cc
@@ -3490,6 +3490,10 @@ expand_asm_stmt (gasm *stmt)
 	= gen_rtx_ASM_INPUT_loc (input_mode[i],
 				 constraints[i + noutputs],
 				 locus);
+      /* This insn correspond to a user-level asm stmt, but this use of
+	 ASM_INPUT doesn't, it's just (quoting rtl.def) "a convenient way to
+	 hold a string".  Mark it artificial.  */
+      ASM_INPUT_ARTIFICIAL_P (ASM_OPERANDS_INPUT_CONSTRAINT_EXP (body, i)) = 1;
     }
 
   /* Copy labels to the vector.  */
diff --git a/gcc/config/nvptx/nvptx.cc b/gcc/config/nvptx/nvptx.cc
index 87efc23bd96a..93df3f309d18 100644
--- a/gcc/config/nvptx/nvptx.cc
+++ b/gcc/config/nvptx/nvptx.cc
@@ -5442,8 +5442,9 @@ gen_comment (const char *s)
   size_t len = strlen (ASM_COMMENT_START) + strlen (sep) + strlen (s) + 1;
   char *comment = (char *) alloca (len);
   snprintf (comment, len, "%s%s%s", ASM_COMMENT_START, sep, s);
-  return gen_rtx_ASM_INPUT_loc (VOIDmode, ggc_strdup (comment),
-				DECL_SOURCE_LOCATION (cfun->decl));
+  rtx asm_input = gen_rtx_ASM_INPUT (VOIDmode, ggc_strdup (comment));
+  ASM_INPUT_ARTIFICIAL_P (asm_input) = 1;
+  return asm_input;
 }
 
 /* Initialize all declared regs at function entry.
diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
index f4404d7abe33..ac8c28d98b94 100644
--- a/gcc/emit-rtl.cc
+++ b/gcc/emit-rtl.cc
@@ -447,6 +447,9 @@ gen_blockage (void)
 {
   rtx x = gen_rtx_ASM_INPUT (VOIDmode, "");
   MEM_VOLATILE_P (x) = true;
+  /* This ASM_INPUT doesn't correspond to a user-level asm stmt.  Mark it
+     artificial.  */
+  ASM_INPUT_ARTIFICIAL_P (x) = 1;
   return x;
 }
 #endif
diff --git a/gcc/final.cc b/gcc/final.cc
index a9868861bd2c..fee512869482 100644
--- a/gcc/final.cc
+++ b/gcc/final.cc
@@ -2642,15 +2642,21 @@ final_scan_insn_1 (rtx_insn *insn, FILE *file, int optimize_p ATTRIBUTE_UNUSED,
 	    if (string[0])
 	      {
 		expanded_location loc;
+		bool unknown_loc_p
+		  = ASM_INPUT_SOURCE_LOCATION (body) == UNKNOWN_LOCATION;
 
-		app_enable ();
-		loc = expand_location (ASM_INPUT_SOURCE_LOCATION (body));
-		if (*loc.file && loc.line)
-		  fprintf (asm_out_file, "%s %i \"%s\" 1\n",
-			   ASM_COMMENT_START, loc.line, loc.file);
+		if (!ASM_INPUT_ARTIFICIAL_P (body))
+		  app_enable ();
+		if (!unknown_loc_p)
+		  {
+		    loc = expand_location (ASM_INPUT_SOURCE_LOCATION (body));
+		    if (*loc.file && loc.line)
+		      fprintf (asm_out_file, "%s %i \"%s\" 1\n",
+			       ASM_COMMENT_START, loc.line, loc.file);
+		  }
 		fprintf (asm_out_file, "\t%s\n", string);
 #if HAVE_AS_LINE_ZERO
-		if (*loc.file && loc.line)
+		if (!unknown_loc_p && *loc.file && loc.line)
 		  fprintf (asm_out_file, "%s 0 \"\" 2\n", ASM_COMMENT_START);
 #endif
 	      }
diff --git a/gcc/rtl.h b/gcc/rtl.h
index 9df2fab622e7..3d7cc2be45c4 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -325,6 +325,7 @@ struct GTY((desc("0"), tag("0"),
      1 in a VALUE is SP_BASED_VALUE_P in cselib.cc.
      1 in a SUBREG generated by LRA for reload insns.
      1 in a REG if this is a static chain register.
+     1 in an ASM_INPUT if it is compiler-generated.
      Dumped as "/j" in RTL dumps.  */
   unsigned int jump : 1;
   /* In a CODE_LABEL, part of the two-bit alternate entry field.
@@ -2592,6 +2593,8 @@ do {								        \
 #define ASM_OPERANDS_LABEL(RTX, N) XCVECEXP (RTX, 5, N, ASM_OPERANDS)
 #define ASM_OPERANDS_SOURCE_LOCATION(RTX) XCUINT (RTX, 6, ASM_OPERANDS)
 #define ASM_INPUT_SOURCE_LOCATION(RTX) XCUINT (RTX, 1, ASM_INPUT)
+#define ASM_INPUT_ARTIFICIAL_P(RTX) \
+  (RTL_FLAG_CHECK1 ("ASM_INPUT_ARTIFICIAL_P", (RTX), ASM_INPUT)->jump)
 
 /* 1 if RTX is a mem that is statically allocated in read-only memory.  */
 #define MEM_READONLY_P(RTX) \