[PATCH][final] Handle compiler-generated asm insn

public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed

* [PATCH][final] Handle compiler-generated asm insn
@ 2022-02-22 13:55 Tom de Vries
  2022-03-09 12:50 ` [PING][PATCH][final] " Tom de Vries
  0 siblings, 1 reply; 8+ messages in thread
From: Tom de Vries @ 2022-02-22 13:55 UTC (permalink / raw)
  To: gcc-patches

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?

Thanks,
- Tom

[final] Handle compiler-generated asm insn

gcc/ChangeLog:

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

	PR rtl-optimization/104596
	* config/nvptx/nvptx.cc (gen_comment): Use gen_rtx_ASM_INPUT instead
	of gen_rtx_ASM_INPUT_loc.
	* final.cc (final_scan_insn_1): Handle
	ASM_INPUT_SOURCE_LOCATION == UNKNOWN_LOCATION.

---
 gcc/config/nvptx/nvptx.cc |  3 +--
 gcc/final.cc              | 17 +++++++++++------
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/gcc/config/nvptx/nvptx.cc b/gcc/config/nvptx/nvptx.cc
index 858789e6df7..4124c597f24 100644
--- a/gcc/config/nvptx/nvptx.cc
+++ b/gcc/config/nvptx/nvptx.cc
@@ -5381,8 +5381,7 @@ 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),
-				cfun->function_start_locus);
+  return gen_rtx_ASM_INPUT (VOIDmode, ggc_strdup (comment));
 }
 
 /* Initialize all declared regs at function entry.
diff --git a/gcc/final.cc b/gcc/final.cc
index a9868861bd2..e6443ef7a4f 100644
--- a/gcc/final.cc
+++ b/gcc/final.cc
@@ -2642,15 +2642,20 @@ 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 (!unknown_loc_p)
+		  {
+		    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);
+		  }
 		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.file && loc.line)
 		  fprintf (asm_out_file, "%s 0 \"\" 2\n", ASM_COMMENT_START);
 #endif
 	      }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PING][PATCH][final] Handle compiler-generated asm insn
  2022-02-22 13:55 [PATCH][final] Handle compiler-generated asm insn Tom de Vries
@ 2022-03-09 12:50 ` Tom de Vries
  2022-03-17 15:09   ` [PING^2][PATCH][final] " Tom de Vries
  0 siblings, 1 reply; 8+ messages in thread
From: Tom de Vries @ 2022-03-09 12:50 UTC (permalink / raw)
  To: gcc-patches

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.

Thanks,
- Tom

> [final] Handle compiler-generated asm insn
> 
> gcc/ChangeLog:
> 
> 2022-02-21  Tom de Vries  <tdevries@suse.de>
> 
> 	PR rtl-optimization/104596
> 	* config/nvptx/nvptx.cc (gen_comment): Use gen_rtx_ASM_INPUT instead
> 	of gen_rtx_ASM_INPUT_loc.
> 	* final.cc (final_scan_insn_1): Handle
> 	ASM_INPUT_SOURCE_LOCATION == UNKNOWN_LOCATION.
> 
> ---
>   gcc/config/nvptx/nvptx.cc |  3 +--
>   gcc/final.cc              | 17 +++++++++++------
>   2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/gcc/config/nvptx/nvptx.cc b/gcc/config/nvptx/nvptx.cc
> index 858789e6df7..4124c597f24 100644
> --- a/gcc/config/nvptx/nvptx.cc
> +++ b/gcc/config/nvptx/nvptx.cc
> @@ -5381,8 +5381,7 @@ 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),
> -				cfun->function_start_locus);
> +  return gen_rtx_ASM_INPUT (VOIDmode, ggc_strdup (comment));
>   }
>   
>   /* Initialize all declared regs at function entry.
> diff --git a/gcc/final.cc b/gcc/final.cc
> index a9868861bd2..e6443ef7a4f 100644
> --- a/gcc/final.cc
> +++ b/gcc/final.cc
> @@ -2642,15 +2642,20 @@ 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 (!unknown_loc_p)
> +		  {
> +		    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);
> +		  }
>   		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.file && loc.line)
>   		  fprintf (asm_out_file, "%s 0 \"\" 2\n", ASM_COMMENT_START);
>   #endif
>   	      }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PING^2][PATCH][final] Handle compiler-generated asm insn
  2022-03-09 12:50 ` [PING][PATCH][final] " Tom de Vries
@ 2022-03-17 15:09   ` Tom de Vries
  2022-03-21  7:58     ` Richard Biener
  0 siblings, 1 reply; 8+ messages in thread
From: Tom de Vries @ 2022-03-17 15:09 UTC (permalink / raw)
  To: gcc-patches

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.

Thanks,
- Tom

>> [final] Handle compiler-generated asm insn
>>
>> gcc/ChangeLog:
>>
>> 2022-02-21  Tom de Vries  <tdevries@suse.de>
>>
>>     PR rtl-optimization/104596
>>     * config/nvptx/nvptx.cc (gen_comment): Use gen_rtx_ASM_INPUT instead
>>     of gen_rtx_ASM_INPUT_loc.
>>     * final.cc (final_scan_insn_1): Handle
>>     ASM_INPUT_SOURCE_LOCATION == UNKNOWN_LOCATION.
>>
>> ---
>>   gcc/config/nvptx/nvptx.cc |  3 +--
>>   gcc/final.cc              | 17 +++++++++++------
>>   2 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/gcc/config/nvptx/nvptx.cc b/gcc/config/nvptx/nvptx.cc
>> index 858789e6df7..4124c597f24 100644
>> --- a/gcc/config/nvptx/nvptx.cc
>> +++ b/gcc/config/nvptx/nvptx.cc
>> @@ -5381,8 +5381,7 @@ 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),
>> -                cfun->function_start_locus);
>> +  return gen_rtx_ASM_INPUT (VOIDmode, ggc_strdup (comment));
>>   }
>>   /* Initialize all declared regs at function entry.
>> diff --git a/gcc/final.cc b/gcc/final.cc
>> index a9868861bd2..e6443ef7a4f 100644
>> --- a/gcc/final.cc
>> +++ b/gcc/final.cc
>> @@ -2642,15 +2642,20 @@ 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 (!unknown_loc_p)
>> +          {
>> +            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);
>> +          }
>>           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.file && loc.line)
>>             fprintf (asm_out_file, "%s 0 \"\" 2\n", ASM_COMMENT_START);
>>   #endif
>>             }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PING^2][PATCH][final] Handle compiler-generated asm insn
  2022-03-17 15:09   ` [PING^2][PATCH][final] " Tom de Vries
@ 2022-03-21  7:58     ` Richard Biener
  2022-03-21 11:50       ` Tom de Vries
  0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2022-03-21  7:58 UTC (permalink / raw)
  To: Tom de Vries, Alexandre Oliva; +Cc: GCC Patches

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.

Usually targets use UNSPECs to emit compiler-generated "asm"
instructions.  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?)

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?  If not maybe there's an unused
bit on ASMs we can enable this way.  IIRC some of the Ada
hardening GIMPLE passes also emit ASMs that could 'benefit'
from this.

Richard.

> Thanks,
> - Tom
>
> >> [final] Handle compiler-generated asm insn
> >>
> >> gcc/ChangeLog:
> >>
> >> 2022-02-21  Tom de Vries  <tdevries@suse.de>
> >>
> >>     PR rtl-optimization/104596
> >>     * config/nvptx/nvptx.cc (gen_comment): Use gen_rtx_ASM_INPUT instead
> >>     of gen_rtx_ASM_INPUT_loc.
> >>     * final.cc (final_scan_insn_1): Handle
> >>     ASM_INPUT_SOURCE_LOCATION == UNKNOWN_LOCATION.
> >>
> >> ---
> >>   gcc/config/nvptx/nvptx.cc |  3 +--
> >>   gcc/final.cc              | 17 +++++++++++------
> >>   2 files changed, 12 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/gcc/config/nvptx/nvptx.cc b/gcc/config/nvptx/nvptx.cc
> >> index 858789e6df7..4124c597f24 100644
> >> --- a/gcc/config/nvptx/nvptx.cc
> >> +++ b/gcc/config/nvptx/nvptx.cc
> >> @@ -5381,8 +5381,7 @@ 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),
> >> -                cfun->function_start_locus);
> >> +  return gen_rtx_ASM_INPUT (VOIDmode, ggc_strdup (comment));
> >>   }
> >>   /* Initialize all declared regs at function entry.
> >> diff --git a/gcc/final.cc b/gcc/final.cc
> >> index a9868861bd2..e6443ef7a4f 100644
> >> --- a/gcc/final.cc
> >> +++ b/gcc/final.cc
> >> @@ -2642,15 +2642,20 @@ 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 (!unknown_loc_p)
> >> +          {
> >> +            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);
> >> +          }
> >>           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.file && loc.line)
> >>             fprintf (asm_out_file, "%s 0 \"\" 2\n", ASM_COMMENT_START);
> >>   #endif
> >>             }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PING^2][PATCH][final] Handle compiler-generated asm insn
  2022-03-21  7:58     ` Richard Biener
@ 2022-03-21 11:50       ` Tom de Vries
  2022-03-21 13:49         ` Richard Biener
  0 siblings, 1 reply; 8+ messages in thread
From: Tom de Vries @ 2022-03-21 11:50 UTC (permalink / raw)
  To: Richard Biener, Alexandre Oliva; +Cc: GCC Patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PING^2][PATCH][final] Handle compiler-generated asm insn
  2022-03-21 11:50       ` Tom de Vries
@ 2022-03-21 13:49         ` Richard Biener
  2022-03-21 16:14           ` Tom de Vries
  0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2022-03-21 13:49 UTC (permalink / raw)
  To: Tom de Vries; +Cc: Alexandre Oliva, GCC Patches

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
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.

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).

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

But yes, this was kind-of what I meant.

Any considerations from others?

Thanks,
Richard.

>
> Thanks,
> - Tom

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PING^2][PATCH][final] Handle compiler-generated asm insn
  2022-03-21 13:49         ` Richard Biener
@ 2022-03-21 16:14           ` Tom de Vries
  2022-07-10  3:38             ` Jeff Law
  0 siblings, 1 reply; 8+ messages in thread
From: Tom de Vries @ 2022-03-21 16:14 UTC (permalink / raw)
  To: Richard Biener; +Cc: Alexandre Oliva, GCC Patches

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PING^2][PATCH][final] Handle compiler-generated asm insn
  2022-03-21 16:14           ` Tom de Vries
@ 2022-07-10  3:38             ` Jeff Law
  0 siblings, 0 replies; 8+ messages in thread
From: Jeff Law @ 2022-07-10  3:38 UTC (permalink / raw)
  To: gcc-patches, Tom de Vries



On 3/21/2022 10:14 AM, Tom de Vries via Gcc-patches wrote:
> 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
>
> 0015-final-Handle-compiler-generated-asm-insn.patch
>
> [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.
This looks pretty reasonable to me.  The only part that wasn't clear was 
the setting of ASM_INPUT_ARTIFICIAL_P in cfgexpand.cc, but I trust that 
it was needed and reasonable.

If you still want to go forward with this Tom, go ahead.

jeff

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-07-10  3:38 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-22 13:55 [PATCH][final] Handle compiler-generated asm insn Tom de Vries
2022-03-09 12:50 ` [PING][PATCH][final] " Tom de Vries
2022-03-17 15:09   ` [PING^2][PATCH][final] " Tom de Vries
2022-03-21  7:58     ` Richard Biener
2022-03-21 11:50       ` Tom de Vries
2022-03-21 13:49         ` Richard Biener
2022-03-21 16:14           ` Tom de Vries
2022-07-10  3:38             ` Jeff Law

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).