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

[Richard Biener <richard.guenther@gmail.com>]

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