Re: [PATCH V2] Clean up loop-closed PHIs after loop finalize

[Jiufu Guo <guojiufu@linux.ibm.com>]

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?

gcc/ChangeLog
2020-10-11  Jiufu Guo   <guojiufu@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   <guojiufu@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"
 
 \f
 /* 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
+      && 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)
+    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);
+	      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);
+
+		  /* 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;
+		}
+	      else
+		gsi_next (&gsi);
+	    }
+    }
+
+  return cfg_altered;
+}
-- 
2.25.1

Richard Biener <rguenther@suse.de> writes:

> 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.
>> >