From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 8B502399E07D for ; Wed, 9 Jun 2021 20:18:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8B502399E07D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 159KHZYD017785; Wed, 9 Jun 2021 15:17:36 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 159KHZlb017784; Wed, 9 Jun 2021 15:17:35 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Wed, 9 Jun 2021 15:17:35 -0500 From: Segher Boessenkool To: "Kewen.Lin" Cc: GCC Patches , Bill Schmidt Subject: Re: [PATCH/RFC] combine: Tweak the condition of last_set invalidation Message-ID: <20210609201735.GJ18427@gate.crashing.org> References: <6bcd32fa-d0ef-b136-ddd9-92a1d21f60af@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6bcd32fa-d0ef-b136-ddd9-92a1d21f60af@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, TXREP, T_SPF_HELO_PERMERROR, T_SPF_PERMERROR autolearn=no 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: Wed, 09 Jun 2021 20:18:37 -0000 Hi! On Wed, Dec 16, 2020 at 04:49:49PM +0800, Kewen.Lin wrote: > Currently we have the check: > > if (!insn > || (value && rsp->last_set_table_tick >= label_tick_ebb_start)) > rsp->last_set_invalid = 1; > > which means if we want to record some value for some reg and > this reg got refered before in a valid scope, If we already know it is *set* in this same extended basic block. Possibly by the same instruction btw. > we invalidate the > set of reg (last_set_invalid to 1). It avoids to find the wrong > set for one reg reference, such as the case like: > > ... op regX // this regX could find wrong last_set below > regX = ... // if we think this set is valid > ... op regX Yup, exactly. > But because of retry's existence, the last_set_table_tick could > be set by some later reference insns, but we see it's set due > to retry on the set (for that reg) insn again, such as: > > insn 1 > insn 2 > > regX = ... --> (a) > ... op regX --> (b) > > insn 3 > > // assume all in the same BB. > > Assuming we combine 1, 2 -> 3 sucessfully and replace them as two > (3 insns -> 2 insns), This will delete insn 1 and write the combined result to insns 2 and 3. > retrying from insn1 or insn2 again: Always 2, but your point remains valid. > it will scan insn (a) again, the below condition holds for regX: > > (value && rsp->last_set_table_tick >= label_tick_ebb_start) > > it will mark this set as invalid set. But actually the > last_set_table_tick here is set by insn (b) before retrying, so it > should be safe to be taken as valid set. Yup. > This proposal is to check whether the last_set_table safely happens > after the current set, make the set still valid if so. > Full SPEC2017 building shows this patch gets more sucessful combines > from 1902208 to 1902243 (trivial though). Do you have some example, or maybe even a testcase? :-) > + /* Record the luid of the insn whose expression involving register n. */ > + > + int last_set_table_luid; "Record the luid of the insn for which last_set_table_tick was set", right? > -static void update_table_tick (rtx); > +static void update_table_tick (rtx, int); Please remove this declaration instead, the function is not used until after its actual definition :-) > @@ -13243,7 +13247,21 @@ update_table_tick (rtx x) > for (r = regno; r < endregno; r++) > { > reg_stat_type *rsp = ®_stat[r]; > - rsp->last_set_table_tick = label_tick; > + if (rsp->last_set_table_tick >= label_tick_ebb_start) > + { > + /* Later references should not have lower ticks. */ > + gcc_assert (label_tick >= rsp->last_set_table_tick); This should be obvious, but checking it won't hurt, okay. > + /* Should pick up the lowest luid if the references > + are in the same block. */ > + if (label_tick == rsp->last_set_table_tick > + && rsp->last_set_table_luid > insn_luid) > + rsp->last_set_table_luid = insn_luid; Why? Is it conservative for the check you will do later? Please spell this out, it is crucial! > @@ -13359,7 +13378,10 @@ record_value_for_reg (rtx reg, rtx_insn *insn, rtx value) > > /* Mark registers that are being referenced in this value. */ > if (value) > - update_table_tick (value); > + { > + gcc_assert (insn); > + update_table_tick (value, DF_INSN_LUID (insn)); > + } Don't add that assert please. If you really want one it should come right at the start of the function, not 60 lines later :-) Looks good if I understood this correctly :-) Segher