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 00F54385E83A for ; Fri, 6 Nov 2020 09:55:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 00F54385E83A 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 0E021AC0C; Fri, 6 Nov 2020 09:55:11 +0000 (UTC) Date: Fri, 6 Nov 2020 10:55:10 +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] Clean up loop-closed PHIs at loopdone pass In-Reply-To: <643c465403d11f033d072ab66224b21e@linux.vnet.ibm.com> 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.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, 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, 06 Nov 2020 09:55:14 -0000 On Fri, 6 Nov 2020, Jiufu Guo wrote: > On 2020-11-05 21:43, Richard Biener wrote: > > Hi Richard, > > Thanks for your comments and suggestions! > > > On Thu, Nov 5, 2020 at 2:19 PM guojiufu via Gcc-patches > > wrote: > >> > >> In PR87473, there are discussions about loop-closed PHIs which > >> are generated for loop optimization passes. It would be helpful > >> to clean them up after loop optimization is done, then this may > >> simplify some jobs of following passes. > >> This patch introduces a cheaper way to propagate them out in > >> pass_tree_loop_done. > >> > >> This patch passes bootstrap and regtest on ppc64le. Is this ok for trunk? > > > > Huh, I think this is somewhat useless work, the PHIs won't survive for long > > and you certainly cannot expect degenerate PHIs to not occur anyway. > > After `loopdone` pass, those loop-closed-PHIs will still live ~10 passes > (veclower, switchlower, slsr...) till the next `copyprop` pass. > It would be helpful to those passes if we can eliminate those degenerated PHIs > in a cheaper way. As you mentioned in > https://gcc.gnu.org/legacy-ml/gcc-patches/2018-10/msg00834.html > > We know vrp/dom may generate some degenerated PHIS, and then we have > `copyprop` > was added after each vrp/dom pair to propagate out those PHIs. Likely, I > think for loop-closed PHIs, we may also eliminate them once they are not > needed. > > > > You probably can replace propagate_rhs_into_lhs by the > > existing replace_uses_by function. You're walking loop exits > > Yes, replace_uses_by + remove_phi_node would be a good implementation > propagate_rhs_into_lhs. > > > Thanks! > > > after loop_optimizer_finalize () - that's wasting work. If you want to > > avoid inconsistent state and we really want to go with this I suggest > > to instead add a flag to loop_optimizer_finalize () as to whether to > > propagate out LC PHI nodes or not and do this from within there. > > Thank you for the suggestion! > You mean adding a flag and in loop_optimizer_finalize, and add code like: > ``` > if (flag_propagate_loop_closed_phi_when_loop_done) > { > loops_state_clear (fn, LOOP_CLOSED_SSA) > clean_up_loop_closed_phis(fn); > } > ``` > > Is this align with your suggestions? Yeah. > One concern: function loop_optimizer_finalize is called a lot of places, > while we just need to clean up loop-closed PHIs at GIMPLE loopdone pass. There are quite some other passes rewriting into LC SSA outside of the loop pipeline. [E]VRP for example but also invariant motion. To avoid touching too many places you can default the new argument to false for example. Richard. > Thanks again, > > Jiufu Guo. > > > > > Thanks, > > Richard. > > > >> gcc/ChangeLog > >> 2020-10-05 Jiufu Guo > >> > >> * tree-ssa-loop.h (clean_up_loop_closed_phi): New declaration. > >> * tree-ssa-loop.c (tree_ssa_loop_done): Call > >> clean_up_loop_closed_phi. > >> * tree-ssa-propagate.c (propagate_rhs_into_lhs): New function. > >> > >> gcc/testsuite/ChangeLog > >> 2020-10-05 Jiufu Guo > >> > >> * gcc.dg/tree-ssa/loopclosedphi.c: New test. > >> --- > >> gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c | 21 +++ > >> gcc/tree-ssa-loop.c | 1 + > >> gcc/tree-ssa-loop.h | 1 + > >> gcc/tree-ssa-propagate.c | 120 > >> ++++++++++++++++++ > >> 4 files changed, 143 insertions(+) > >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/loopclosedphi.c > >> > >> 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..d71b757fbca > >> --- /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" } */ > >> + > >> +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-ssa-loop.c b/gcc/tree-ssa-loop.c > >> index 5e8365d4e83..7d680b2f5d2 100644 > >> --- a/gcc/tree-ssa-loop.c > >> +++ b/gcc/tree-ssa-loop.c > >> @@ -530,6 +530,7 @@ tree_ssa_loop_done (void) > >> free_numbers_of_iterations_estimates (cfun); > >> scev_finalize (); > >> loop_optimizer_finalize (); > >> + clean_up_loop_closed_phi (cfun); > >> return 0; > >> } > >> > >> diff --git a/gcc/tree-ssa-loop.h b/gcc/tree-ssa-loop.h > >> index 9e35125e6e8..baa940b9d1e 100644 > >> --- a/gcc/tree-ssa-loop.h > >> +++ b/gcc/tree-ssa-loop.h > >> @@ -67,6 +67,7 @@ public: > >> extern bool for_each_index (tree *, bool (*) (tree, tree *, void *), void > >> *); > >> extern char *get_lsm_tmp_name (tree ref, unsigned n, const char *suffix = > >> NULL); > >> extern unsigned tree_num_loop_insns (class loop *, struct eni_weights *); > >> +extern unsigned clean_up_loop_closed_phi (function *); > >> > >> /* Returns the loop of the statement STMT. */ > >> > >> diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c > >> index 87dbf55fab9..813143852b9 100644 > >> --- a/gcc/tree-ssa-propagate.c > >> +++ b/gcc/tree-ssa-propagate.c > >> @@ -1549,4 +1549,123 @@ propagate_tree_value_into_stmt > >> (gimple_stmt_iterator *gsi, tree val) > >> else > >> gcc_unreachable (); > >> } > >> + > >> +/* Propagate RHS into all uses of LHS (when possible). > >> + > >> + RHS and LHS are derived from STMT, which is passed in solely so > >> + that we can remove it if propagation is successful. */ > >> + > >> +static bool > >> +propagate_rhs_into_lhs (gphi *stmt, tree lhs, tree rhs) > >> +{ > >> + use_operand_p use_p; > >> + imm_use_iterator iter; > >> + gimple_stmt_iterator gsi; > >> + gimple *use_stmt; > >> + bool changed = false; > >> + bool all = true; > >> + > >> + /* 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"); > >> + } > >> + > >> + /* Walk over every use of LHS and try to replace the use with RHS. */ > >> + FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs) > >> + { > >> + /* It is not safe to propagate into below stmts. */ > >> + if (gimple_debug_bind_p (use_stmt) > >> + || (gimple_code (use_stmt) == GIMPLE_ASM > >> + && !may_propagate_copy_into_asm (lhs)) > >> + || (TREE_CODE (rhs) == SSA_NAME > >> + && SSA_NAME_DEF_STMT (rhs) == use_stmt)) > >> + { > >> + all = false; > >> + continue; > >> + } > >> + > >> + /* Dump details. */ > >> + if (dump_file && (dump_flags & TDF_DETAILS)) > >> + { > >> + fprintf (dump_file, " Original statement:"); > >> + print_gimple_stmt (dump_file, use_stmt, 0, dump_flags); > >> + } > >> + > >> + /* Propagate the RHS into this use of the LHS. */ > >> + FOR_EACH_IMM_USE_ON_STMT (use_p, iter) > >> + propagate_value (use_p, rhs); > >> + > >> + /* Propagation may expose new operands to the renamer. */ > >> + update_stmt (use_stmt); > >> + > >> + /* If variable index is replaced with a constant, then > >> + update the invariant flag for ADDR_EXPRs. */ > >> + if (gimple_assign_single_p (use_stmt) > >> + && TREE_CODE (gimple_assign_rhs1 (use_stmt)) == ADDR_EXPR) > >> + recompute_tree_invariant_for_addr_expr (gimple_assign_rhs1 > >> (use_stmt)); > >> + > >> + /* Dump details. */ > >> + if (dump_file && (dump_flags & TDF_DETAILS)) > >> + { > >> + fprintf (dump_file, " Updated statement:"); > >> + print_gimple_stmt (dump_file, use_stmt, 0, dump_flags); > >> + } > >> + > >> + changed = true; > >> + } > >> + > >> + /* Remove the degenerate PHI node. */ > >> + if (all) > >> + { > >> + gsi = gsi_for_stmt (stmt); > >> + remove_phi_node (&gsi, true); > >> + } > >> + > >> + return changed; > >> +} > >> + > >> +/* 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; > >> + > >> + /* Walk over loop in function. */ > >> + FOR_EACH_LOOP_FN (fun, loop, 0) > >> + { > >> + /* Check each exit edge 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); > >> + lhs = gimple_phi_result (phi); > >> + > >> + /* Advance the iterator before stmt is removed. */ > >> + gsi_next (&gsi); > >> + > >> + if (rhs && !virtual_operand_p (lhs) > >> + && may_propagate_copy (lhs, rhs)) > >> + cfg_altered |= propagate_rhs_into_lhs (phi, lhs, rhs); > >> + } > >> + } > >> + > >> + return cfg_altered; > >> +} > >> -- > >> 2.25.1 > >> > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imend