On Tue, 30 Jan 2018, Jakub Jelinek wrote:
> 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?
Yes.
> OT, why is loop->num signed rather than unsigned?
History. Like so many others (block->index, etc.). Possibly to
allow better optimization of loops over those indexes (relying
on undefined overflow).
Richard.
> 2018-01-30 Richard Biener <rguent...@suse.de>
> Jakub Jelinek <ja...@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
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
