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 1C9E83858415; Wed, 27 Oct 2021 07:44:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1C9E83858415 Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id DA205280F28; Wed, 27 Oct 2021 09:44:54 +0200 (CEST) Date: Wed, 27 Oct 2021 09:44:54 +0200 From: Jan Hubicka To: Richard Biener Cc: Xionghu Luo , gcc-patches@gcc.gnu.org, segher@kernel.crashing.org, wschmidt@linux.ibm.com, linkw@gcc.gnu.org, dje.gcc@gmail.com Subject: Re: [PATCH v2 1/4] Fix loop split incorrect count and probability Message-ID: <20211027074454.GC57414@kam.mff.cuni.cz> References: <20211027063448.1844771-1-luoxhu@linux.ibm.com> <20211027063448.1844771-2-luoxhu@linux.ibm.com> <20211027070732.GA57414@kam.mff.cuni.cz> <55673non-925q-qq24-n1n-q8nro6q752n@fhfr.qr> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-2 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <55673non-925q-qq24-n1n-q8nro6q752n@fhfr.qr> User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-12.2 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: Wed, 27 Oct 2021 07:44:57 -0000 > On Wed, 27 Oct 2021, Jan Hubicka wrote: > > > > > > > gcc/ChangeLog: > > > > > > * tree-ssa-loop-split.c (split_loop): Fix incorrect probability. > > > (do_split_loop_on_cond): Likewise. > > > --- > > > gcc/tree-ssa-loop-split.c | 25 ++++++++++++++++--------- > > > 1 file changed, 16 insertions(+), 9 deletions(-) > > > > > > diff --git a/gcc/tree-ssa-loop-split.c b/gcc/tree-ssa-loop-split.c > > > index 3f6ad046623..d30782888f3 100644 > > > --- a/gcc/tree-ssa-loop-split.c > > > +++ b/gcc/tree-ssa-loop-split.c > > > @@ -575,7 +575,11 @@ split_loop (class loop *loop1) > > > stmts2); > > > tree cond = build2 (guard_code, boolean_type_node, guard_init, border); > > > if (!initial_true) > > > - cond = fold_build1 (TRUTH_NOT_EXPR, boolean_type_node, cond); > > > + cond = fold_build1 (TRUTH_NOT_EXPR, boolean_type_node, cond); > > > + > > > + edge true_edge = EDGE_SUCC (bbs[i], 0)->flags & EDGE_TRUE_VALUE > > > + ? EDGE_SUCC (bbs[i], 0) > > > + : EDGE_SUCC (bbs[i], 1); > > > > > > /* Now version the loop, placing loop2 after loop1 connecting > > > them, and fix up SSA form for that. */ > > > @@ -583,10 +587,10 @@ split_loop (class loop *loop1) > > > basic_block cond_bb; > > > > > > class loop *loop2 = loop_version (loop1, cond, &cond_bb, > > > - profile_probability::always (), > > > - profile_probability::always (), > > > - profile_probability::always (), > > > - profile_probability::always (), > > > + true_edge->probability, > > > + true_edge->probability.invert (), > > > + true_edge->probability, > > > + true_edge->probability.invert (), > > > true); > > > > As discussed yesterday, for loop of form > > > > for (...) > > if (cond) > > cond = something(); > > else > > something2 > > > > Split as > > Note that you are missing to conditionalize loop1 execution > on 'cond' (not sure if that makes a difference). You are right - forgot to mention that. Entry conditional makes no difference on scaling stmts inside loop but affects its header and expected trip count. We however need to set up probability of this conditional (and preheader count if it exists) There is no general way to read the probability of this initial conditional from cfg profile. So I guess we are stuck with guessing some arbitrary value. I guess common case is that cond is true first iteration tough and often we can easily see that fromo PHI node initializing the test variable. Other thing that changes is expected number of iterations of the split loops, so we may want to update the exit conditinal probability accordingly... Honza > > > loop1: > if (cond) > > for (...) > > if (true) > > cond = something(); > > if (!cond) > > break > > else > > something2 (); > > > > loop2: > > for (...) > > if (false) > > cond = something(); > > else > > something2 (); > > > > If "if (cond)" has probability p, you want to scale loop1 by p > > and loop2 by 1-p as your patch does, but you need to exclude the basic > > blocks guarded by the condition. > > > > One way is to break out loop_version and implement it inline, other > > option (perhaps leading to less code duplication) is to add argument listing > > basic blocks that should not be scaled, which would be set to both arms > > of the if. > > > > Are there other profile patches of your I should look at? > > Honza > > > gcc_assert (loop2); > > > > > > @@ -1486,10 +1490,10 @@ do_split_loop_on_cond (struct loop *loop1, edge invar_branch) > > > initialize_original_copy_tables (); > > > > > > struct loop *loop2 = loop_version (loop1, boolean_true_node, NULL, > > > - profile_probability::always (), > > > - profile_probability::never (), > > > - profile_probability::always (), > > > - profile_probability::always (), > > > + invar_branch->probability.invert (), > > > + invar_branch->probability, > > > + invar_branch->probability.invert (), > > > + invar_branch->probability, > > > true); > > > if (!loop2) > > > { > > > @@ -1530,6 +1534,9 @@ do_split_loop_on_cond (struct loop *loop1, edge invar_branch) > > > to_loop1->flags |= true_invar ? EDGE_FALSE_VALUE : EDGE_TRUE_VALUE; > > > to_loop2->flags |= true_invar ? EDGE_TRUE_VALUE : EDGE_FALSE_VALUE; > > > > > > + to_loop1->probability = invar_branch->probability.invert (); > > > + to_loop2->probability = invar_branch->probability; > > > + > > > /* Due to introduction of a control flow edge from loop1 latch to loop2 > > > pre-header, we should update PHIs in loop2 to reflect this connection > > > between loop1 and loop2. */ > > > -- > > > 2.27.0.90.geebb51ba8c > > > > > > > -- > Richard Biener > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, > Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)