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