RE: [Revised Patch] to handle perfect loop unrolling in tree_transform_and_unroll_loop

["Bingfeng Mei" <bmei at broadcom dot com>]

Zdenek,
Thanks for comments. 

> This will fail if the loop has more than one exit.  Also, I 
> somewhat miss the point of dealing with
> updating of frequencies and probabilities separately in the 
> case of perfect unrolling (the code
> for the general case should work, possibly with minor tweaks).
Initially, I tried to use the original code. But it doesn't seem
to produce correct frequency/probability for me. Additionally,
it causes ICE for some complex loops (e.g. 20060905-1.c). 

      exit_bb = single_pred (loop->latch);
      new_exit = find_edge (exit_bb, rest);

The latch may have more than one predecessors. I couldn't use
above statements to find exit_bb/exit edge. 

tree_transform_and_unroll_loop/tree_unroll_loop are used only
after determinate_unroll_factor, which checks whether
the loop has single exit through can_unroll_loop_p. 

Cheers,
Bingfeng

> -----Original Message-----
> From: Zdenek Dvorak [mailto:rakdver@kam.mff.cuni.cz] 
> Sent: 06 November 2009 11:08
> To: Bingfeng Mei
> Cc: Jean Christophe Beyler; gcc-patches@gcc.gnu.org
> Subject: Re: [Revised Patch] to handle perfect loop unrolling 
> in tree_transform_and_unroll_loop
> 
> Hi,
> 
> > +/* Check whether the number of iterations can be divided 
> by unrolling
> > +   factor to perform perfect unrolling */
> 
> the arguments of the function should be documented.
> 
> > +static bool
> > +perfect_loop_unrolling_p(struct loop *loop, int factor, 
> unsigned *num_iters)
> 
> Space is missing before (
> 
> > +{
> > +   tree niters = number_of_exit_cond_executions(loop);
> 
> You should use the number of iterations information provided 
> to tree_transform_and_unroll_loop
> via struct tree_niter_desc *desc argument.
> 
> > +   if(niters != NULL_TREE 
> 
> ditto (and everywhere else in the patch, I stop pointing this 
> out here).
> 
> > +     && niters!= chrec_dont_know 
> > +     && TREE_CODE(niters) == INTEGER_CST)
> 
> host_integerp
> 
> > +     {
> > +       *num_iters = tree_low_cst(niters, 1);
> 
> type of num_iters should be unsigned HOST_WIDE_INT. 
> I don't think it is a good idea to return number of 
> iterations this way, as this
> looks rather unrelated to the purpose of this function.
> 
> > +  bool perfect_unrolling = perfect_loop_unrolling_p(loop, 
> factor, &est_niter);
> >  
> > -  est_niter = expected_loop_iterations (loop);
> 
> It seems more natural to use est_niter = 
> expected_loop_iterations (which may differ from the value
> in desc->niter in case that the loop has several exits).
> 
> > +  if (perfect_unrolling)
> > +    {
> > +      new_exit = single_dom_exit (loop);
> 
> This will fail if the loop has more than one exit.  Also, I 
> somewhat miss the point of dealing with
> updating of frequencies and probabilities separately in the 
> case of perfect unrolling (the code
> for the general case should work, possibly with minor tweaks).
> 
> Zdenek
> 
>