From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x52e.google.com (mail-pg1-x52e.google.com [IPv6:2607:f8b0:4864:20::52e]) by sourceware.org (Postfix) with ESMTPS id 73288386C5BC for ; Sun, 10 Jul 2022 03:38:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 73288386C5BC Received: by mail-pg1-x52e.google.com with SMTP id g4so2166169pgc.1 for ; Sat, 09 Jul 2022 20:38:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to; bh=6r6XVPKQV0XB3whkTyQ7qW8C/UkuM0jP0SOPGofws0U=; b=n+gqWjokntpggwbyX9mjcnjifo/WRO9G1csDav9YhUiRWbA+actsVQEhL8vX3NAAui hmWMdL+cOmQFVW4T8zOExjUAmt5/r2nzTXeIGHglEt6oFVtP3mzD8qVpBK+rYJrpSjxI 82eWPDbvwnstMszQQEARfZoJG2aN/5h5QPwkfYx32o3Ym1iNc19d5P+i8bNBVa0dW/YF O1YzFI75BziHFveBeeMs9fzWlMsnXf2QUGgMnW75kngmfBFC6zHqqbbCuTCiPD2PTMpS W+ymEGJfiAsEekG75QG3pVHqdldsY8cg5nzpRBZ04m+3B0ymU8OovTdrFvTuv/zgVQKz bwlw== X-Gm-Message-State: AJIora8lzNob0Ax7mz3QBGcr6Q6mVq0faNAysstg+VGmXpfwVPT6Q7+4 oM6+BntsMU2iBHIV2TxAEKIY8ivZrHg= X-Google-Smtp-Source: AGRyM1uj2Voe4IRydAE5JWC3zdAe1NiHAXl5eVLNcMDysgmIrH+Rn3XIJxDx18MYIL/pRm6gaudVLQ== X-Received: by 2002:a63:eb03:0:b0:412:b1d6:94ca with SMTP id t3-20020a63eb03000000b00412b1d694camr10124942pgh.468.1657424306007; Sat, 09 Jul 2022 20:38:26 -0700 (PDT) Received: from [172.31.0.204] (c-73-63-24-84.hsd1.ut.comcast.net. [73.63.24.84]) by smtp.gmail.com with ESMTPSA id o5-20020a17090aac0500b001efbc3ad105sm1966555pjq.54.2022.07.09.20.38.25 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 09 Jul 2022 20:38:25 -0700 (PDT) Message-ID: <181bc882-6d91-0f74-41f5-0c7835e1c76d@gmail.com> Date: Sat, 9 Jul 2022 21:38:24 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PING^2][PATCH][final] Handle compiler-generated asm insn Content-Language: en-US To: gcc-patches@gcc.gnu.org, Tom de Vries References: <20220222135513.GA1591@delia.home> <2efa0fec-aea3-3b9e-61f2-7f0f332668e7@suse.de> <3a85ff98-5966-2f83-1f68-75daf29d9793@suse.de> From: Jeff Law In-Reply-To: <3a85ff98-5966-2f83-1f68-75daf29d9793@suse.de> X-Spam-Status: No, score=-1.0 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, HTML_MESSAGE, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 10 Jul 2022 03:38:30 -0000 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 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 > > 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 > > 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