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

[Jeff Law <jeffreyalaw@gmail.com>]

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