From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id 151463A1B40C for ; Fri, 13 Nov 2020 08:18:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 151463A1B40C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rguenther@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id D74EAAD07; Fri, 13 Nov 2020 08:18:46 +0000 (UTC) Date: Fri, 13 Nov 2020 09:18:46 +0100 (CET) From: Richard Biener Sender: rguenther@c653.arch.suse.de To: Jiufu Guo cc: Richard Biener , GCC Patches , Segher Boessenkool , Bill Schmidt , David Edelsohn Subject: Re: [PATCH V2] Clean up loop-closed PHIs after loop finalize In-Reply-To: Message-ID: References: <20201105131836.1043501-1-guojiufu@linux.ibm.com> <643c465403d11f033d072ab66224b21e@linux.vnet.ibm.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H3, 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: Fri, 13 Nov 2020 08:18:49 -0000 On Wed, 11 Nov 2020, Jiufu Guo wrote: > > Thanks a lot for the sugguestion from previous mails. > The patch was updated accordingly. > > This updated patch propagates loop-closed PHIs them out after > loop_optimizer_finalize under a new introduced flag. At some cases, > to clean up loop-closed PHIs would save efforts of optimization passes > after loopdone. > > This patch passes bootstrap and regtest on ppc64le. Is this ok for trunk? Comments below > gcc/ChangeLog > 2020-10-11 Jiufu Guo > > * common.opt (flag_clean_up_loop_closed_phi): New flag. > * loop-init.c (loop_optimizer_finalize): Check > flag_clean_up_loop_closed_phi and call clean_up_loop_closed_phi. > * tree-cfgcleanup.h (clean_up_loop_closed_phi): New declare. > * tree-ssa-propagate.c (clean_up_loop_closed_phi): New function. > > gcc/testsuite/ChangeLog > 2020-10-11 Jiufu Guo > > * gcc.dg/tree-ssa/loopclosedphi.c: New test. > > --- > gcc/common.opt | 4 ++ > gcc/loop-init.c | 8 +++ > gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c | 21 +++++++ > gcc/tree-cfgcleanup.h | 1 + > gcc/tree-ssa-propagate.c | 61 +++++++++++++++++++ > 5 files changed, 95 insertions(+) > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c > > diff --git a/gcc/common.opt b/gcc/common.opt > index 7e789d1c47f..f0d7b74d7ad 100644 > --- a/gcc/common.opt > +++ b/gcc/common.opt > @@ -1141,6 +1141,10 @@ fchecking= > Common Joined RejectNegative UInteger Var(flag_checking) > Perform internal consistency checkings. > > +fclean-up-loop-closed-phi > +Common Report Var(flag_clean_up_loop_closed_phi) Optimization Init(0) > +Clean up loop-closed PHIs after loop optimization done. > + > fcode-hoisting > Common Report Var(flag_code_hoisting) Optimization > Enable code hoisting. > diff --git a/gcc/loop-init.c b/gcc/loop-init.c > index 401e5282907..05804759ac9 100644 > --- a/gcc/loop-init.c > +++ b/gcc/loop-init.c > @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see > #include "tree-ssa-loop-niter.h" > #include "loop-unroll.h" > #include "tree-scalar-evolution.h" > +#include "tree-cfgcleanup.h" > > > /* Apply FLAGS to the loop state. */ > @@ -145,6 +146,13 @@ loop_optimizer_finalize (struct function *fn) > > free_numbers_of_iterations_estimates (fn); > > + if (flag_clean_up_loop_closed_phi Sorry if there was miscommunication but I've not meant to add a new user-visible flag but instead a flag argument to loop_optimizer_finalize (as said, you can default it to false to only need to change the one in fini_loops) > + && loops_state_satisfies_p (fn, LOOP_CLOSED_SSA)) > + { > + clean_up_loop_closed_phi (fn); > + loops_state_clear (fn, LOOP_CLOSED_SSA); > + } > + > /* If we should preserve loop structure, do not free it but clear > flags that advanced properties are there as we are not preserving > that in full. */ > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c b/gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c > new file mode 100644 > index 00000000000..ab22a991935 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c > @@ -0,0 +1,21 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O3 -fno-tree-ch -w -fdump-tree-loopdone-details -fclean-up-loop-closed-phi" } */ > + > +void > +t6 (int qz, int wh) > +{ > + int jl = wh; > + > + while (1.0 * qz / wh < 1) > + { > + qz = wh * (wh + 2); > + > + while (wh < 1) > + jl = 0; > + } > + > + while (qz < 1) > + qz = jl * wh; > +} > + > +/* { dg-final { scan-tree-dump-times "Replacing" 2 "loopdone"} } */ > diff --git a/gcc/tree-cfgcleanup.h b/gcc/tree-cfgcleanup.h > index 6ff6726bfe4..9e368d63709 100644 > --- a/gcc/tree-cfgcleanup.h > +++ b/gcc/tree-cfgcleanup.h > @@ -26,5 +26,6 @@ extern bool cleanup_tree_cfg (unsigned = 0); > extern bool fixup_noreturn_call (gimple *stmt); > extern bool delete_unreachable_blocks_update_callgraph (cgraph_node *dst_node, > bool update_clones); > +extern unsigned clean_up_loop_closed_phi (function *); > > #endif /* GCC_TREE_CFGCLEANUP_H */ > diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c > index 87dbf55fab9..a3bfe36c733 100644 > --- a/gcc/tree-ssa-propagate.c > +++ b/gcc/tree-ssa-propagate.c > @@ -1549,3 +1549,64 @@ propagate_tree_value_into_stmt (gimple_stmt_iterator *gsi, tree val) > else > gcc_unreachable (); > } > + > +/* Check exits of each loop in FUN, walk over loop closed PHIs in > + each exit basic block and propagate degenerate PHIs. */ > + > +unsigned > +clean_up_loop_closed_phi (function *fun) > +{ > + unsigned i; > + edge e; > + gphi *phi; > + tree rhs; > + tree lhs; > + gphi_iterator gsi; > + struct loop *loop; > + bool cfg_altered = false; > + > + /* Check dominator info before get loop-close PHIs from loop exits. */ > + if (dom_info_state (CDI_DOMINATORS) != DOM_OK) Why? > + return 0; > + > + /* Walk over loop in function. */ > + FOR_EACH_LOOP_FN (fun, loop, 0) > + { > + /* Check each exit edege of loop. */ > + auto_vec exits = get_loop_exit_edges (loop); > + FOR_EACH_VEC_ELT (exits, i, e) > + if (single_pred_p (e->dest)) > + /* Walk over loop-closed PHIs. */ > + for (gsi = gsi_start_phis (e->dest); !gsi_end_p (gsi);) > + { > + phi = gsi.phi (); > + rhs = degenerate_phi_result (phi); You have a single predecessor, thus the result is always degenerate. > + lhs = gimple_phi_result (phi); > + > + if (rhs && may_propagate_copy (lhs, rhs)) > + { > + gimple_stmt_iterator psi = gsi; > + /* Advance the iterator before stmt is removed. */ > + gsi_next (&gsi); remove_phi_node should take care of this, you shouldn't need this (just do not advance the iterator when you remove the PHI node). > + > + /* Dump details. */ > + if (dump_file && (dump_flags & TDF_DETAILS)) > + { > + fprintf (dump_file, " Replacing '"); > + print_generic_expr (dump_file, lhs, dump_flags); > + fprintf (dump_file, "' with '"); > + print_generic_expr (dump_file, rhs, dump_flags); > + fprintf (dump_file, "'\n"); > + } > + > + replace_uses_by (lhs, rhs); > + remove_phi_node (&psi, true); > + cfg_altered = true; in the end the return value is unused but I think we should avoid altering the CFG since doing so requires it to be cleaned up for unreachable blocks. That means to open-code replace_uses_by as imm_use_iterator imm_iter; use_operand_p use; gimple *stmt; FOR_EACH_IMM_USE_STMT (stmt, imm_iter, name) { FOR_EACH_IMM_USE_ON_STMT (use, imm_iter) replace_exp (use, val); update_stmt (stmt); } Thanks, Richard. > + } > + else > + gsi_next (&gsi); > + } > + } > + > + return cfg_altered; > +} > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imend