From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x633.google.com (mail-pl1-x633.google.com [IPv6:2607:f8b0:4864:20::633]) by sourceware.org (Postfix) with ESMTPS id 482C13858D20; Mon, 12 Jun 2023 20:54:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 482C13858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-pl1-x633.google.com with SMTP id d9443c01a7336-1b3d800f671so9442155ad.0; Mon, 12 Jun 2023 13:54:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686603273; x=1689195273; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=TEwc5nQMs84mfwawvw2S05J8bUj+dLr/OWpsUL9t3tM=; b=brPumPJ2fGBLkYmKnVf7X1mBASuTcThqAK2hQobBELyOn663NUNdHd+BxF3KAZbG1w bJM6wkj0Oi4tYg6NQ3Fd0uHbKUBwtMjUu+yIIsMkqFbxCshkxsh0CIRGtKQKczzJ9C0E iF7c0CWzXJlRrSIvTLO7OgXb80kLeb0WY3NLGNpdWfVSKCBfxIVGTBbqZbpukiBhSebP dHqe6/cj2TUkjAkoTuucxrGIpwpDV9YspjjsqBfgHXYmS4k04dhvPExed7/I/enX7HWj i3V71vifXjxjjxhGAhq+nRE1AXPyPj2Ex3xhTFN9MRcEXPAD2Sf889fhNZrF5IkEBPWi Wy6A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686603273; x=1689195273; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=TEwc5nQMs84mfwawvw2S05J8bUj+dLr/OWpsUL9t3tM=; b=Ky0OhSuo9epwRiYjf6M8VuvIL3MFtS/WHLzigxhtatDxOZ56a46R+M9bIvv02fdrYy a8Hb1IuXhSj4K5gsV8FH+wbF1eIw2RM2zCKT3gQdGzYJaB5SCUbxjGcO7s0B/pGfvYbk T6jwgHbYT/YK+4C0734PPMvRQ78rN2r7u2lU7vBS7zfaP7p2Vx53edEL8MTeWUOwC1eB ARAnyV7dqNALhhArZSqf44YhFZACBSwqB7xfm0UquaAX7BZ/H083k2/E4C7ipyRg++dK 31/PSAdbCh6IG9b+cA8QhyKfSnFs63NSUGvgMYattZgOPl66huiQXQjRmVrwGpDZXOF7 9y+A== X-Gm-Message-State: AC+VfDz4DiXk7RRtHenRyMfg8UadGovi9TOPRcyKu2PHzYikptBOZ8gl M4bjC6igD+Q4hfcW6swUnjg= X-Google-Smtp-Source: ACHHUZ6bTXweZ2WnNCd0eoAVr7kZDSep5CAxp/6ubrGbfMqqkPp/4qPl911GeVqP+mAgKw6actRHmg== X-Received: by 2002:a17:903:44a:b0:1b3:dbb1:757 with SMTP id iw10-20020a170903044a00b001b3dbb10757mr2002660plb.47.1686603272938; Mon, 12 Jun 2023 13:54:32 -0700 (PDT) Received: from [172.31.0.109] ([136.36.130.248]) by smtp.gmail.com with ESMTPSA id j1-20020a170902c3c100b001ac5896e96esm4006991plj.207.2023.06.12.13.54.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 12 Jun 2023 13:54:32 -0700 (PDT) Message-ID: Date: Mon, 12 Jun 2023 14:54:30 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [PATCH] Make sure SCALAR_INT_MODE_P before invoke try_const_anchors Content-Language: en-US To: Jiufu Guo , Richard Biener Cc: Richard Sandiford , gcc-patches@gcc.gnu.org, segher@kernel.crashing.org, dje.gcc@gmail.com, linkw@gcc.gnu.org, bergner@linux.ibm.com References: <20230609052847.2128612-1-guojiufu@linux.ibm.com> <56dbba43adda001d1668c29e8024c85d@linux.ibm.com> <7nwn0dgfvj.fsf@ltcden2-lp1.aus.stglabs.ibm.com> <7no7lohklj.fsf@ltcden2-lp1.aus.stglabs.ibm.com> <7na5x5dyjx.fsf@ltcden2-lp1.aus.stglabs.ibm.com> From: Jeff Law In-Reply-To: <7na5x5dyjx.fsf@ltcden2-lp1.aus.stglabs.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-2.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_SHORT,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 List-Id: On 6/11/23 23:44, Jiufu Guo wrote: > Richard Biener writes: > >> On Fri, 9 Jun 2023, Jiufu Guo wrote: >> >>> >>> Hi, >>> >>> Richard Biener writes: >>> >>>> On Fri, 9 Jun 2023, Jiufu Guo wrote: >>>> >>>>> >>>>> Hi, >>>>> >>>>> Richard Biener writes: >>>>> >>>>>> On Fri, 9 Jun 2023, Richard Sandiford wrote: >>>>>> >>>>>>> guojiufu writes: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 2023-06-09 16:00, Richard Biener wrote: >>>>>>>>> On Fri, 9 Jun 2023, Jiufu Guo wrote: >>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> >>> ... >>>>>>>>>> >>>>>>>>>> This patch is raised when drafting below one. >>>>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html. >>>>>>>>>> With that patch, "{[%1:DI]=0;} stack_tie" with BLKmode runs into >>>>>>>>>> try_const_anchors, and hits the assert/ice. >>>>>>>>>> >>>>>>>>>> Boostrap and regtest pass on ppc64{,le} and x86_64. >>>>>>>>>> Is this ok for trunk? >>>>>>>>> >>>>>>>>> Iff the correct fix at all (how can a CONST_INT have BLKmode?) then >>>>>>>>> I suggest to instead fix try_const_anchors to change >>>>>>>>> >>>>>>>>> /* CONST_INT is used for CC modes, but we should leave those alone. >>>>>>>>> */ >>>>>>>>> if (GET_MODE_CLASS (mode) == MODE_CC) >>>>>>>>> return NULL_RTX; >>>>>>>>> >>>>>>>>> gcc_assert (SCALAR_INT_MODE_P (mode)); >>>>>>>>> >>>>>>>>> to >>>>>>>>> >>>>>>>>> /* CONST_INT is used for CC modes, leave any non-scalar-int mode >>>>>>>>> alone. */ >>>>>>>>> if (!SCALAR_INT_MODE_P (mode)) >>>>>>>>> return NULL_RTX; >>>>>>>>> >>>>>>>> >>>>>>>> This is also able to fix this issue. there is a "Punt on CC modes" >>>>>>>> patch >>>>>>>> to return NULL_RTX in try_const_anchors. >>>>>>>> >>>>>>>>> but as said I wonder how we arrive at a BLKmode CONST_INT and whether >>>>>>>>> we should have fended this off earlier. Can you share more complete >>>>>>>>> RTL of that stack_tie? >>>>>>>> >>>>>>>> >>>>>>>> (insn 15 14 16 3 (parallel [ >>>>>>>> (set (mem/c:BLK (reg/f:DI 1 1) [1 A8]) >>>>>>>> (const_int 0 [0])) >>>>>>>> ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie} >>>>>>>> (nil)) >>>>>>>> >>>>>>>> It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])". >>>>>>> >>>>>>> I'm not convinced this is correct RTL. (unspec:BLK [(const_int 0)] ...) >>>>>>> would be though. It's arguably more accurate too, since the effect >>>>>>> on the stack locations is unspecified rather than predictable. >>>>>> >>>>>> powerpc seems to be the only port with a stack_tie that's not >>>>>> using an UNSPEC RHS. >>>>> In rs6000.md, it is >>>>> >>>>> ; This is to explain that changes to the stack pointer should >>>>> ; not be moved over loads from or stores to stack memory. >>>>> (define_insn "stack_tie" >>>>> [(match_parallel 0 "tie_operand" >>>>> [(set (mem:BLK (reg 1)) (const_int 0))])] >>>>> "" >>>>> "" >>>>> [(set_attr "length" "0")]) >>>>> >>>>> This would be just an placeholder insn, and acts as the comments. >>>>> UNSPEC_ would works like other targets. While, I'm wondering >>>>> the concerns on "set (mem:BLK (reg 1)) (const_int 0)". >>>>> MODEs between SET_DEST and SET_SRC? >>>> >>>> I don't think the issue is the mode but the issue is that >>>> the patter as-is says some memory is zeroed while that's not >>>> actually true (not specifying a size means we can't really do >>>> anything with this MEM, but still). Using an UNSPEC avoids >>>> implying anything for the stored value. >>>> >>>> Of course I think a MEM SET_DEST without a specified size is bougs >>>> as well, but there's larger precedent for this... >>> >>> Thanks for your kindly comments! >>> Using "(set (mem:BLK (reg 1)) (const_int 0))" here, may because this >>> insn does not generate real thing (not a real store and no asm code), >>> may like barrier. >>> >>> While I agree that, using UNSPEC may be more clear to avoid mis-reading. >> >> Btw, another way to avoid the issue in CSE is to make it not process >> (aka record anything for optimization) for SET from MEMs with >> !MEM_SIZE_KNOWN_P > > Thanks! Yes, this would make sense. > Then, there are two ideas(patches) to handle this issue: > Which one would be preferable? This one (from compiling time aspect)? > > And maybe, the changes in rs6000 stack_tie through using unspec > can be a standalone enhancement besides cse patch. I'd tend to lean more towards fixing the rs6000 backend. It's basically lying to the rest of the compiler and when it presents passes with something like (set (mem:BLK) (const_int 0)) It's largely inviting the generic bits to treat it like a memory store, when in fact it's something significantly different. I don't think the CSE patch is wrong or a bad idea, more that it's just papering over a problem caused by an odd chunk of RTL created by the PPC backend. jeff