Re: [PATCH] Fix SCEV ICE during cunroll (PR tree-optimization/84111)

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Jan 30, 2018 at 08:59:43AM +0100, Richard Biener wrote:
> The issue is that
> 
> static bool
> tree_unroll_loops_completely_1 (bool may_increase_size, bool unroll_outer,
>                                 bitmap father_bbs, struct loop *loop)
> {
>   struct loop *loop_father;
>   bool changed = false;
>   struct loop *inner;
>   enum unroll_level ul;
> 
>   /* Process inner loops first.  */
>   for (inner = loop->inner; inner != NULL; inner = inner->next)
>     changed |= tree_unroll_loops_completely_1 (may_increase_size,
>                                                unroll_outer, father_bbs,
>                                                inner);
> 
> when recursing tree_unroll_loops_completely_1 appends unrolled
> bodies and loops contained therein in the loop->inner chain.  We
> specifically do _not_ allow processing of outer loops here due
> to not up-to-date SSA form and then iterate the unrolling process
> instead:
> 
>   /* If we changed an inner loop we cannot process outer loops in this
>      iteration because SSA form is not up-to-date.  Continue with
>      siblings of outer loops instead.  */
>   if (changed)
>     return true;
> 
> so I think the proper fix is to avoid visiting loops that were added
> in the above iteration over loop->inner.  For example by collecting
> the loop->inner chain in a vec and then iterating over that (much
> like FOR_EACH_LOOP does).  Or rely on the fact that loop->num of new
> loops will be >= number_of_loops () [number_of_loops () is really
> mis-named, would be a good time to add max_loop_num () or so]

So like this if it passes bootstrap/regtest?

OT, why is loop->num signed rather than unsigned?

2018-01-30  Richard Biener  <rguenther@suse.de>
	    Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/84111
	* tree-ssa-loop-ivcanon.c (tree_unroll_loops_completely_1): Skip
	inner loops added during recursion, as they don't have up-to-date
	SSA form.

	* gcc.c-torture/compile/pr84111.c: New test.

--- gcc/tree-ssa-loop-ivcanon.c.jj	2018-01-29 14:05:46.689153783 +0100
+++ gcc/tree-ssa-loop-ivcanon.c	2018-01-30 09:30:21.932069737 +0100
@@ -1373,13 +1373,17 @@ tree_unroll_loops_completely_1 (bool may
   bool changed = false;
   struct loop *inner;
   enum unroll_level ul;
+  unsigned num = number_of_loops (cfun);
 
-  /* Process inner loops first.  */
+  /* Process inner loops first.  Don't walk loops added by the recursive
+     calls because SSA form is not up-to-date.  They can be handled in the
+     next iteration.  */
   for (inner = loop->inner; inner != NULL; inner = inner->next)
-    changed |= tree_unroll_loops_completely_1 (may_increase_size,
-					       unroll_outer, father_bbs,
-					       inner);
- 
+    if ((unsigned) inner->num < num)
+      changed |= tree_unroll_loops_completely_1 (may_increase_size,
+						 unroll_outer, father_bbs,
+						 inner);
+
   /* If we changed an inner loop we cannot process outer loops in this
      iteration because SSA form is not up-to-date.  Continue with
      siblings of outer loops instead.  */
--- gcc/testsuite/gcc.c-torture/compile/pr84111.c.jj	2018-01-29 20:38:10.236528914 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr84111.c	2018-01-29 20:37:53.227522763 +0100
@@ -0,0 +1,31 @@
+/* PR tree-optimization/84111 */
+
+void
+foo (int x, int y, int z)
+{
+  int a = 0;
+  int *b = &x;
+
+  while (a < 1)
+    {
+      int c = y;
+      *b = x;
+ lab:
+      for (a = 0; a < 36; ++a)
+        {
+          *b = 0;
+          if (x != 0)
+            y = 0;
+          while (c < 1)
+	    ;
+        }
+    }
+  if (z < 33)
+    {
+      b = (int *) 0;
+      ++y;
+      ++z;
+      if (x / *b != 0)
+        goto lab;
+    }
+}


	Jakub