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
> > <gcc-patches@gcc.gnu.org> 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 <guoji...@linux.ibm.com>
> >>
> >> * 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 <guoji...@linux.ibm.com>
> >>
> >> * 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<edge> 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 <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend
