Re: [PATCH] sh, loop iv optim

[Zdenek Dvorak <rakdver at kam dot mff dot cuni dot cz>]

Hello,

> This patch provides an option, -f[no-]invert-ivs, that allows a loop 
> exit to be tested against zero. This is very useful for architectures 
> having a decrement and test instruction.

we already have this optimization, see loop-doloop.c.  You should find
out why it does not work for you on sh.  Nevertheless, some more comments:

> Also, this optimisation exhibited a bug with the initialization of the 
> number of iterations in loop-iv.c:iv_number_of_iterations that was 
> handling the difference between pointers as a unsigned entity.

That is correct, any differences computed in iv_number_of_iterations are
unsigned.

> as such 
> the logical right shift was losing the sign bit. This case happened with 
> a loop like
> while (end > start) with an initial value of start > end.

You need to check that noloop_assumptions is false before using
the result of iv_number_of_iterations.  I.e.,

> In this case 
> the loop's number of iterations should be negative. To fix it I force 
> the desc->signed_p to one when making a difference between two pointers 
> and use a div instead of udiv. The code is less optimized but should be 
> safe.

this change is wrong.

> 	* loop-iv.c (iv_number_of_iterations): Make diffs between pointers 
> 	signed.
> 	(exit_on_zero_cond): New function.
> 	(need_guarded_loop): New function.
> 	(may_invert_loop): New function.
> 	(invert_loops): New function.

New passes should be put to separate files.

> +  for (i = 0; i < loop->num_nodes; i++)
> +    {
> +      basic_block bb;
> +
> +      bb = bbs[i];
> +
> +      /* Find the first conditional jump in the loop that depends on an iv.  */

You would probably want to check here whether there is a loop exit edge
from this bb, here.

> +      cond = get_condition (BB_END (bb), &at, false, false);
> +      if (cond)
> +	{
> +	  /* we want only the iv used as condition */
> +	  for (i = 0; i < 2; i++)
> +	    {
> +	      rtx op = XEXP (cond, i);
> +	      
> +	      if (!CONSTANT_P (op) && iv_analyze_biv (op, &iv) 
> +		  && iv.step != const0_rtx && !iv.first_special)
> +		{
> +		  edge_iterator ei;
> +		  edge e;
> +
> +		  FOR_EACH_EDGE (e, ei, bb->succs)
> +		    {
> +		      basic_block bbe = e->dest;
> +		      
> +		      /* if this is an exit jump, take it when new iv
> +			 reaches 0.  */
> +		      if (LABEL_P (BB_HEAD (bbe))
> +			  && BB_HEAD (bbe) == JUMP_LABEL (BB_END (bb)))
> +			{
> +			  *edgep = e;
> +			  return cond;
> +			}

This is weird, I am not quite sure what are you trying to achieve.  If
you are trying to find the edge taken when the condition is true, you
should look for the edge without EDGE_FALLTHRU flag.

> +/* Check if the number of iterations that will be decremented could be negative
> +   and thus the first decrement and test would miss the first exit.  */
> +
> +static bool
> +need_guarded_loop (struct loop *loop, rtx cond)
> +{
> +  struct niter_desc *desc;
> +  enum rtx_code code = GET_CODE (cond);
> +
> +  desc = get_simple_loop_desc (loop);
> +
> +  if (CONSTANT_P (desc->niter_expr) || code == EQ || code == NE)
> +    return false;
> +
> +  return true;
> +}

You want to check desc->noloop_assumptions here, instead.

Zdenek