Re: [patch] Fix PR/18179 and use get_inner_reference in vectorizer: part 2

[Richard Henderson <rth at redhat dot com>]

On Tue, Dec 14, 2004 at 08:54:48AM +0200, Ira Rosen wrote:
> +   /* Stop conditions:
> +      1. Constant.  */
> +   if (host_integerp (expr, 1))

You want TREE_CONSTANT, surely.

> +       tree access_fn = analyze_scalar_evolution (loop, expr);
> +       if (access_fn == chrec_dont_know /* No access_fn.  */
> + 
> + 	  /* Not enough information: may be not loop invariant.  
> + 	     E.g., for a[b[i]], we get a[D], where D=b[i]. EXPR is D, its 
> + 	     initial_condition is D, but it depends on i - loop's induction
> + 	     variable.  */	  
> + 	  || (((init = initial_condition_in_loop_num (access_fn, loop->num))
> + 	       == expr)
> + 	      && !IS_EMPTY_STMT (def_stmt = SSA_NAME_DEF_STMT (init))
> + 	      && flow_bb_inside_loop_p (loop, bb_for_stmt (def_stmt)))
> + 
> + 	  /* Evolution is not constant.  */
> + 	  || ((evolution = evolution_part_in_loop_num (access_fn, loop->num))
> + 	      && !host_integerp (evolution, 0)))
> + 	{
> + 	  *step = NULL_TREE;
> + 	  *misalign = NULL_TREE;
> + 	  *initial_offset = NULL_TREE;
> + 	  return false;	  
> + 	}

Please break up this conditional.  Like so.

	*step = NULL_TREE;
	*misalign = NULL_TREE;
	*initial_offset = NULL_TREE;

Move these to the beginning of the function (at least before the
SSA_NAME conditional, and then the various failure paths out of
the function only need to "return false".

	if (access_fn == chrec_dont_know)
	  return false;

	init = initial_condition_in_loop_num (access_fn, loop->num);
	if (init != expr)
	  {
	    def_stmt = SSA_NAME_DEF_STMT (init);
	    if (!IS_EMPTY_STMT (def_stmt)
		&& flow_bb_inside_loop_p (loop, bb_for_stmt (def_stmt)))
	      return false;
	  }

	evolution = evolution_part_in_loop_num (access_fn, loop->num);
	if (evolution && !host_integerp (evolution, 0))
	  return false;

Resist the temptation to embed assignments inside conditionals.
Three is really just over-the-top too much.

Do you actually want TREE_CONSTANT there at the end too?  Not
having looked at what the interface of evolution_part_in_loop_num
is, I'm not sure.

For the rest, let's just take as written that I'd like you to
re-examine all other ocurrences of host_integerp and defend them.

> +       /* Assumption: in MULT_EXPR the left operand is a variable and the right 
> + 	 operand is a constant. Therefore, LEFT_OFFSET can be either a variable
> + 	 or a constant, and RIGHT_OFFSET is always a constant.  */

On what do you base this assumption?  Arrays of variable sized types
will have no constant operand.  Has something above this point already
discarded this as a possibility?



r~