From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id C2A8E393D03B for ; Fri, 14 May 2021 09:09:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C2A8E393D03B Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5DAD61480; Fri, 14 May 2021 02:09:26 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 87AB43F719; Fri, 14 May 2021 02:09:25 -0700 (PDT) From: Richard Sandiford To: Jakub Jelinek Mail-Followup-To: Jakub Jelinek , Hongtao Liu , Eric Botcazou , Jeff Law , gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Cc: Hongtao Liu , Eric Botcazou , Jeff Law , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] regcprop: Fix another cprop_hardreg bug [PR100342] References: <20210119144514.GA4020736@tucnak> <20210505174446.GU1179226@tucnak> <20210513153736.GK1179226@tucnak> <20210513170102.GQ3748@tucnak> Date: Fri, 14 May 2021 10:09:24 +0100 In-Reply-To: <20210513170102.GQ3748@tucnak> (Jakub Jelinek's message of "Thu, 13 May 2021 19:01:02 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-6.6 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org 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: Fri, 14 May 2021 09:09:28 -0000 Jakub Jelinek writes: > On Thu, May 13, 2021 at 05:37:36PM +0200, Jakub Jelinek wrote: >> So, do you want something like (I've deleted the old comment as I think >> the new one is enough, but am open to keep both) the patch below, where >> it REG_CAN_CHANGE_MODE_P is false, we punt (return), otherwise call >> set_value_regno? >> Am not sure if those REG_CAN_CHANGE_MODE_P arguments is what you want >> though. > > Oops, missing !, meant following which works on 11 branch for the testcase: > > 2021-05-13 Jakub Jelinek > > PR rtl-optimization/100342 > * regcprop.c (copy_value): When copying a source reg in a wider > mode than it has recorded for the value, adjust recorded destination > mode too or punt if !REG_CAN_CHANGE_MODE_P. > > * gcc.target/i386/pr100342.c: New test. > > --- gcc/regcprop.c.jj 2021-03-23 10:21:07.176447920 +0100 > +++ gcc/regcprop.c 2021-05-13 17:36:46.443192451 +0200 > @@ -358,34 +358,25 @@ copy_value (rtx dest, rtx src, struct va > else if (sn > hard_regno_nregs (sr, vd->e[sr].mode)) > return; > > - /* It is not safe to link DEST into the chain if SRC was defined in some > - narrower mode M and if M is also narrower than the mode of the first > - register in the chain. For example: > - (set (reg:DI r1) (reg:DI r0)) > - (set (reg:HI r2) (reg:HI r1)) > - (set (reg:SI r3) (reg:SI r2)) //Should be a new chain start at r3 > - (set (reg:SI r4) (reg:SI r1)) > - (set (reg:SI r5) (reg:SI r4)) > - > - the upper part of r3 is undefined. If we added it to the chain, > - it may be used to replace r5, which has defined upper bits. > - See PR98694 for details. > - > - [A] partial_subreg_p (vd->e[sr].mode, GET_MODE (src)) > - [B] partial_subreg_p (vd->e[sr].mode, vd->e[vd->e[sr].oldest_regno].mode) > - Condition B is added to to catch optimization opportunities of > - > - (set (reg:HI R1) (reg:HI R0)) > - (set (reg:SI R2) (reg:SI R1)) // [A] > - (set (reg:DI R3) (reg:DI R2)) // [A] > - (set (reg:SI R4) (reg:SI R[0-3])) > - (set (reg:HI R5) (reg:HI R[0-4])) > - > - in which all registers have only 16 defined bits. */ > - else if (partial_subreg_p (vd->e[sr].mode, GET_MODE (src)) > - && partial_subreg_p (vd->e[sr].mode, > - vd->e[vd->e[sr].oldest_regno].mode)) > - return; > + /* If a narrower value is copied using wider mode, the upper bits > + are undefined (could be e.g. a former paradoxical subreg). Signal > + in that case we've only copied value using the narrower mode. > + Consider: > + (set (reg:DI r14) (mem:DI ...)) > + (set (reg:QI si) (reg:QI r14)) > + (set (reg:DI bp) (reg:DI r14)) > + (set (reg:DI r14) (const_int ...)) > + (set (reg:DI dx) (reg:DI si)) > + (set (reg:DI si) (const_int ...)) > + (set (reg:DI dx) (reg:DI bp)) > + The last set is not redundant, while the low 8 bits of dx are already > + equal to low 8 bits of bp, the other bits are undefined. */ > + else if (partial_subreg_p (vd->e[sr].mode, GET_MODE (src))) > + { > + if (!REG_CAN_CHANGE_MODE_P (sr, GET_MODE (src), vd->e[sr].mode)) > + return; LGTM, but for extra safety, I think we also want: || !REG_CAN_CHANGE_MODE_P (dr, vd->e[sr].mode, GET_MODE (dst)) i.e. we're effectively converting the source from the wider mode to the narrower mode, doing a move, and then converting the narrow mode back to the wider mode. OK with that change, thanks. Richard