On 3/21/22 14:49, Richard Biener wrote: > On Mon, Mar 21, 2022 at 12:50 PM Tom de Vries 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 >>> 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