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

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

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