Re: Fix PR78060, PR78061, PR78088

[Richard Biener <richard dot guenther at gmail dot com>]

On October 25, 2016 5:03:15 PM GMT+02:00, Michael Matz <matz@suse.de> wrote:
>Hi,
>
>these are ICEs by the loop splitting pass.  Two problems: using 
>inconsistent types for operations and not dealing correctly with
>certain 
>loop forms.
>
>First (78060 and 78088): I initially used an ideosyncratic way of 
>calculating the new loop bound, needing several type switches that I 
>haven't implemented correctly.  Reordering the order of evaluations
>makes 
>this easier and fixes the ICEs.
>
>Second (78061): I need to know the exit values of all loop phis.  In 
>simple loop forms (where the single exit is after the loop body) that's
>
>easy: it's the same as the value on the backedge.  If the exit is in
>the 
>middle of a loop that's not the case.  This mattered only for virtual
>ops.  
>All other values already needed a loop-closed phi-node before or in the
>
>latch block, which made it not empty which rejected the loop form. 
>I've 
>added a predicate that explicitely checks that the exit values are
>easily 
>computable (i.e. that the back edge value is indeed also the exit
>value).
>
>Together regstrapped on x86-64, all languages + ada.  Okay for trunk?

OK.

Thanks,
Richard.

>
>Ciao,
>Michael.
>
>	PR tree-optimization/78060
>	PR tree-optimization/78061
>	PR tree-optimization/78088
>	* tree-ssa-loop-split.c (easy_exit_values): New function.
>	(tree_ssa_split_loops): Use it.
>	(compute_new_first_bound): Change order of operations,
>	fix invalid use of types.
>
>testsuite:
>	* g++.dg/pr78060.C: New test.
>	* gfortran.dg/pr78061.f: New test.
>	* g++.dg/pr78088.C: New test.
>
>commit 10cbbd81f96d5183979dae647b77ee804179780e
>Author: Michael Matz <matz@suse.de>
>Date:   Mon Oct 24 17:14:08 2016 +0200
>
>    pr78060 pr78061 pr78088
>
>diff --git a/gcc/testsuite/g++.dg/pr78060.C
>b/gcc/testsuite/g++.dg/pr78060.C
>new file mode 100644
>index 0000000..d6cc7b3
>--- /dev/null
>+++ b/gcc/testsuite/g++.dg/pr78060.C
>@@ -0,0 +1,14 @@
>+// PR tree-optimization/78060
>+// { dg-do compile }
>+// { dg-options "-O3 -fsplit-loops" }
>+class A {
>+public:
>+  template <typename T2> int &operator[](T2);
>+};
>+int a;
>+A b;
>+void fn1() {
>+  long c;
>+  for (int l; l < c; ++l)
>+    b[l] = l < 2 ?: a;
>+}
>diff --git a/gcc/testsuite/g++.dg/pr78088.C
>b/gcc/testsuite/g++.dg/pr78088.C
>new file mode 100644
>index 0000000..1a5c063
>--- /dev/null
>+++ b/gcc/testsuite/g++.dg/pr78088.C
>@@ -0,0 +1,19 @@
>+// PR tree-optimization/78088
>+// { dg-do compile }
>+// { dg-options "-O3 -fsplit-loops" }
>+class A {
>+public:
>+  int m_fn1();
>+};
>+struct B : A {
>+  void m_fn2();
>+};
>+void B::m_fn2() {
>+  long a;
>+  int b, c;
>+  for (;;) {
>+    c = 0;
>+    for (; c < a; ++c, ++b)
>+      b > 0 ? m_fn1() : 0;
>+  }
>+}
>diff --git a/gcc/testsuite/gfortran.dg/pr78061.f
>b/gcc/testsuite/gfortran.dg/pr78061.f
>new file mode 100644
>index 0000000..7e4dd3d
>--- /dev/null
>+++ b/gcc/testsuite/gfortran.dg/pr78061.f
>@@ -0,0 +1,18 @@
>+! { dg-do compile }
>+! { dg-options "-O3 -fsplit-loops" }
>+      SUBROUTINE SSYMM(C)
>+      REAL C(LDC,*)
>+      LOGICAL LSAME
>+      LOGICAL UPPER
>+      IF (LSAME) THEN
>+          DO 170 J = 1,N
>+              DO 140 K = 1,J  
>+                  IF (UPPER) THEN
>+                      END IF
>+  140         CONTINUE
>+              DO 160 K = J + 1,N
>+                  C(I,J) = B(K)
>+  160         CONTINUE
>+  170     CONTINUE
>+      END IF
>+      END
>diff --git a/gcc/tree-ssa-loop-split.c b/gcc/tree-ssa-loop-split.c
>index 53abb36..dac68e6 100644
>--- a/gcc/tree-ssa-loop-split.c
>+++ b/gcc/tree-ssa-loop-split.c
>@@ -190,13 +190,40 @@ find_or_create_guard_phi (struct loop *loop, tree
>guard_iv, affine_iv * /*iv*/)
>   return NULL;
> }
> 
>+/* Returns true if the exit values of all loop phi nodes can be
>+   determined easily (i.e. that connect_loop_phis can determine them).
> */
>+
>+static bool
>+easy_exit_values (struct loop *loop)
>+{
>+  edge exit = single_exit (loop);
>+  edge latch = loop_latch_edge (loop);
>+  gphi_iterator psi;
>+
>+  /* Currently we regard the exit values as easy if they are the same
>+     as the value over the backedge.  Which is the case if the
>definition
>+     of the backedge value dominates the exit edge.  */
>+  for (psi = gsi_start_phis (loop->header); !gsi_end_p (psi); gsi_next
>(&psi))
>+    {
>+      gphi *phi = psi.phi ();
>+      tree next = PHI_ARG_DEF_FROM_EDGE (phi, latch);
>+      basic_block bb;
>+      if (TREE_CODE (next) == SSA_NAME
>+	  && (bb = gimple_bb (SSA_NAME_DEF_STMT (next)))
>+	  && !dominated_by_p (CDI_DOMINATORS, exit->src, bb))
>+	return false;
>+    }
>+
>+  return true;
>+}
>+
> /* This function updates the SSA form after connect_loops made a new
>    edge NEW_E leading from LOOP1 exit to LOOP2 (via in intermediate
>    conditional).  I.e. the second loop can now be entered either
>    via the original entry or via NEW_E, so the entry values of LOOP2
>    phi nodes are either the original ones or those at the exit
>    of LOOP1.  Insert new phi nodes in LOOP2 pre-header reflecting
>-   this.  */
>+   this.  The loops need to fulfill easy_exit_values().  */
> 
> static void
> connect_loop_phis (struct loop *loop1, struct loop *loop2, edge new_e)
>@@ -383,37 +410,37 @@ compute_new_first_bound (gimple_seq *stmts,
>struct tree_niter_desc *niter,
> 			    TREE_TYPE (controlbase),
> 			    controlbase, controlstep);
> 
>-  /* Compute beg-guard_init.  */
>+  /* Compute end-beg.  */
>+  gimple_seq stmts2;
>+  tree end = force_gimple_operand (niter->bound, &stmts2,
>+					true, NULL_TREE);
>+  gimple_seq_add_seq_without_update (stmts, stmts2);
>   if (POINTER_TYPE_P (TREE_TYPE (enddiff)))
>     {
>-      tree tem = gimple_convert (stmts, sizetype, guard_init);
>+      tree tem = gimple_convert (stmts, sizetype, enddiff);
>       tem = gimple_build (stmts, NEGATE_EXPR, sizetype, tem);
>       enddiff = gimple_build (stmts, POINTER_PLUS_EXPR,
> 			      TREE_TYPE (enddiff),
>-			      enddiff, tem);
>+			      end, tem);
>     }
>   else
>     enddiff = gimple_build (stmts, MINUS_EXPR, TREE_TYPE (enddiff),
>-			    enddiff, guard_init);
>+			    end, enddiff);
> 
>-  /* Compute end-(beg-guard_init).  */
>-  gimple_seq stmts2;
>-  tree newbound = force_gimple_operand (niter->bound, &stmts2,
>-					true, NULL_TREE);
>-  gimple_seq_add_seq_without_update (stmts, stmts2);
>-
>-  if (POINTER_TYPE_P (TREE_TYPE (enddiff))
>-      || POINTER_TYPE_P (TREE_TYPE (newbound)))
>+  /* Compute guard_init + (end-beg).  */
>+  tree newbound;
>+  enddiff = gimple_convert (stmts, TREE_TYPE (guard_init), enddiff);
>+  if (POINTER_TYPE_P (TREE_TYPE (guard_init)))
>     {
>       enddiff = gimple_convert (stmts, sizetype, enddiff);
>       enddiff = gimple_build (stmts, NEGATE_EXPR, sizetype, enddiff);
>       newbound = gimple_build (stmts, POINTER_PLUS_EXPR,
>-			       TREE_TYPE (newbound),
>-			       newbound, enddiff);
>+			       TREE_TYPE (guard_init),
>+			       guard_init, enddiff);
>     }
>   else
>-    newbound = gimple_build (stmts, MINUS_EXPR, TREE_TYPE (enddiff),
>-			     newbound, enddiff);
>+    newbound = gimple_build (stmts, PLUS_EXPR, TREE_TYPE (guard_init),
>+			     guard_init, enddiff);
> 
>   /* Depending on the direction of the IVs the new bound for the first
>      loop is the minimum or maximum of old bound and border.
>@@ -449,7 +476,6 @@ compute_new_first_bound (gimple_seq *stmts, struct
>tree_niter_desc *niter,
> 			       build_int_cst (type2, addbound));
>     }
> 
>-  newbound = gimple_convert (stmts, TREE_TYPE (border), newbound);
>   tree newend = gimple_build (stmts, minmax, TREE_TYPE (border),
> 			      border, newbound);
>   return newend;
>@@ -615,6 +641,7 @@ tree_ssa_split_loops (void)
> 	     original exit before.  */
> 	  && empty_block_p (loop->latch)
> 	  && !optimize_loop_for_size_p (loop)
>+	  && easy_exit_values (loop)
> 	  && number_of_iterations_exit (loop, single_exit (loop), &niter,
> 					false, true)
> 	  && niter.cmp != ERROR_MARK