From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id B1F373854821 for ; Thu, 13 May 2021 15:37:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B1F373854821 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-528-LsB8ZcvePCedNExW5uoq6g-1; Thu, 13 May 2021 11:37:39 -0400 X-MC-Unique: LsB8ZcvePCedNExW5uoq6g-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B86EE1019632; Thu, 13 May 2021 15:37:38 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-114-59.ams2.redhat.com [10.36.114.59]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 21F1F3807; Thu, 13 May 2021 15:37:37 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 14DFbatx3327946 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 13 May 2021 17:37:36 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 14DFbavE3327945; Thu, 13 May 2021 17:37:36 +0200 Date: Thu, 13 May 2021 17:37:36 +0200 From: Jakub Jelinek To: Hongtao Liu , Eric Botcazou , Jeff Law , gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Subject: Re: [PATCH] regcprop: Fix another cprop_hardreg bug [PR100342] Message-ID: <20210513153736.GK1179226@tucnak> Reply-To: Jakub Jelinek References: <20210119144514.GA4020736@tucnak> <20210505174446.GU1179226@tucnak> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, 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: Thu, 13 May 2021 15:37:43 -0000 On Tue, May 11, 2021 at 11:59:24AM +0100, Richard Sandiford via Gcc-patches wrote: > > I wrote the following patch (originally against 10 branch because that is > > where Uros has been debugging it) and bootstrapped/regtested it on 11 > > branch successfully. > > It effectively implements your (2) above; I'm not sure if > > REG_CAN_CHANGE_MODE_P is needed there, because it is already tested in > > find_oldest_value_reg -> maybe_mode_change -> mode_change_ok. > > The REG_CAN_CHANGE_MODE_P test would in this case be for > vd->e[dr].mode → vd->e[sr].mode, rather than oldest_regno's mode. > I'm just worried that: > > (set (reg:HI R1) (reg:HI R0)) > (set (reg:SI R2) (reg:SI R1)) > > isn't equivalent to: > > (set (reg:HI R1) (reg:HI R0)) > (set (reg:HI R2) (reg:HI R1)) > > if REG_CAN_CHANGE_MODE_P is false for either the R2 or R1 change. > If we pretend that it is when building the chain then there's a > risk of GIGO when using it in find_oldest_value_reg. > > (Although in this case SI and HI are both valid for R1, > REG_CAN_CHANGE_MODE_P might still be false if the HI bits are > not in the low 16 bits of the SI. That's unlikely in this case, > but a similar thing can happen for vector modes or multi-register modes.) > > I'm not saying the patch is wrong. I just wanted to clarify > why I thought the check might be needed. 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. --- gcc/regcprop.c.jj 2021-03-23 10:21:07.176447920 +0100 +++ gcc/regcprop.c 2021-05-13 17:31:39.940519855 +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; + set_value_regno (dr, vd->e[sr].mode, vd); + } /* Link DR at the end of the value chain used by SR. */ Jakub