From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id C33DA385840C; Mon, 13 Dec 2021 10:24:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C33DA385840C Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 62F44280E95; Mon, 13 Dec 2021 11:24:50 +0100 (CET) Date: Mon, 13 Dec 2021 11:24:50 +0100 From: Jan Hubicka To: Xionghu Luo Cc: gcc-patches@gcc.gnu.org, segher@kernel.crashing.org, wschmidt@linux.ibm.com, linkw@gcc.gnu.org, dje.gcc@gmail.com Subject: Re: [PATCH 1/3] loop-invariant: Don't move cold bb instructions to preheader in RTL Message-ID: <20211213102450.GK50931@kam.mff.cuni.cz> References: <20211208055416.1415283-1-luoxhu@linux.ibm.com> <20211208055416.1415283-2-luoxhu@linux.ibm.com> <20211213091441.GC79706@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211213091441.GC79706@kam.mff.cuni.cz> User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, GIT_PATCH_0, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Mon, 13 Dec 2021 10:24:54 -0000 > > gcc/ChangeLog: > > > > * loop-invariant.c (find_invariants_bb): Check profile count > > before motion. > > (find_invariants_body): Add argument. > > --- > > gcc/loop-invariant.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c > > index 5eee2e5c9f8..c61c8612fae 100644 > > --- a/gcc/loop-invariant.c > > +++ b/gcc/loop-invariant.c > > @@ -1183,9 +1183,14 @@ find_invariants_insn (rtx_insn *insn, bool always_reached, bool always_executed) > > call. */ > > > > static void > > -find_invariants_bb (basic_block bb, bool always_reached, bool always_executed) > > +find_invariants_bb (class loop *loop, basic_block bb, bool always_reached, > > + bool always_executed) > > { > > rtx_insn *insn; > > + basic_block preheader = loop_preheader_edge (loop)->src; > > + > > + if (preheader->count > bb->count) > > + return; > > Please add a comment explaining the conditional and if possible also a > testcase. Since profile updating and use is sensitive topic and it may > trigger regressions later, it is important to keep track of info why > given tests was added. > > I wonder why the cost model chose to move any invariatns to preheader > why preheader->count > bb->count? Thinking about this more, you want to test: if (!always_executed && preheader->count > bb->count) return; This will rule out some profile misupates. Also the code updating always_reached is worng: if (always_reached && CALL_P (insn) && (RTL_LOOPING_CONST_OR_PURE_CALL_P (insn) || ! RTL_CONST_OR_PURE_CALL_P (insn))) always_reached = false; It misses the case where statement can trhow external exception or volatile asms that can also terminate execution. I bit worry on how often the test will have false positives with guessed profile where earlier loop optimizations reduced trip count below realistic estimate. Honza > > Honza > > > > FOR_BB_INSNS (bb, insn) > > { > > @@ -1214,8 +1219,7 @@ find_invariants_body (class loop *loop, basic_block *body, > > unsigned i; > > > > for (i = 0; i < loop->num_nodes; i++) > > - find_invariants_bb (body[i], > > - bitmap_bit_p (always_reached, i), > > + find_invariants_bb (loop, body[i], bitmap_bit_p (always_reached, i), > > bitmap_bit_p (always_executed, i)); > > } > > > > -- > > 2.25.1 > >