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

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

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

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?

Thanks,
- Tom

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

[final] Handle compiler-generated asm insn

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:
- adding new flag ASM_INPUT_ARTIFICIAL_P
- 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.
	* 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/config/nvptx/nvptx.cc |  5 +++--
 gcc/final.cc              | 18 ++++++++++++------
 gcc/rtl.h                 |  3 +++
 3 files changed, 18 insertions(+), 8 deletions(-)

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/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) \