Re: [PATCH 3/3] Loop flattening on loop-SSA.

[Richard Guenther <rguenther at suse dot de>]

On Fri, 5 Nov 2010, Sebastian Pop wrote:

> Hi Richi,
> 
> Thanks for reviewing this.
> >
> > This function should be in cfgloop.c and implemented in simpler
> > form, like
> >
> > void
> > cancel_subloops (struct loop *loop)
> > {
> > ?while (loop->inner)
> > ? ?cancel_loop_tree (loop->inner);
> > }
> >
> 
> Ok I will move this function to cfgloop.c.  However, if I don't think
> we can simplify it further without extra storage: if I write the
> simplified form like this:
> 
> void
> cancel_subloops (struct loop *loop)
> {
>   loop_p x = loop->inner;
> 
>   while (x)
>     {
>       cancel_loop_tree (x);
>       x = x->next;
>     }
> }
> 
> this won't work, as the loop x gets first canceled and then we try to
> access x->next and this will produce a segfault.

Which is why my suggested variant would work, no?

> >> + ? ? ?phi = create_phi_node (var, loop->latch);
> >> + ? ? ?create_new_def_for (gimple_phi_result (phi), phi,
> >> + ? ? ? ? ? ? ? ? ? ? ? gimple_phi_result_ptr (phi));
> >
> > Using create_new_def_for looks very suspicios. ?create_phi_node
> > will already create a new SSA name for you for the result, so
> > it doesn't make any sense to fiddle with the SSA updaters machinery, does
> > it?
> >
> 
> When I wrote this, I was following some other code in if-conversion.
> I will remove this pattern from this file and from if-conversion.

Thanks.

> >> +/* Flattens all the loops of the current function. ?*/
> >> +
> >> +static unsigned int
> >> +tree_loop_flattening (void)
> >> +{
> >> + ?unsigned todo = 0;
> >> + ?loop_p loop;
> >> + ?loop_iterator li;
> >> + ?tree scratch_pad = NULL_TREE;
> >> +
> >> + ?if (number_of_loops () <= 1)
> >> + ? ?return 0;
> >> +
> >> + ?FOR_EACH_LOOP (li, loop, 0)
> >> + ? ?todo |= flatten_loop (loop, &scratch_pad);
> >
> > So we might end up recursively flattening loops (or not, as this
> > walk is in undefined order).
> 
> The order in which we flatten loops does not really matter.
> 
> > ?I'd say you want LI_ONLY_INNERMOST here,
> > or do you really want to flatten all loop trees up to the number
> > of basic blocks specified in the parm? ?I guess not.
> 
> For a given number of basic blocks per flat loop, any loop tree
> traversal will end up with the same loop tree.  If we start walking
> the loop tree from the innermost, we may end up flattening flat loops.

loops with a flattened body you mean?  What I question is the usefullness
of flattening a complete nest.  What I also question is whether
flattening works for outer loops (well - the default # of blocks to
flatten seems to be so low that we never do that and I guess you'll
simply ICE flattening an outer loop if you increase that limit).

Thus, I think you should restrict your self to LI_ONLY_INNERMOST.

Richard.