Re: [PATCH] Fix PR56478

[Jakub Jelinek <jakub at redhat dot com>]

On Thu, Feb 28, 2013 at 07:27:48PM +0100, Marek Polacek wrote:
> The hunk 
> probability = (double) REG_BR_PROB_BASE * compare_count / loop_count;
> in there should be probably handled in the same way.  But I'll handle
> that separately.
> 
> In the first hunk, I did s/int/HOST_WIDE_INT/, because HWI is what
> tree_low_cst returns.  For why that could be a problem, see the 
> Jakub's comment in the PR.

That doesn't help, because it is assigned into
int *loop_step;
*loop_step = step;

IMHO the argument should be tree *loop_step, then you don't need to
convert it back from int to tree.

> --- gcc/predict.c.mp	2013-02-28 17:26:47.950247877 +0100
> +++ gcc/predict.c	2013-02-28 17:26:56.855275792 +0100

> +      /* We want to do the arithmetics on trees (and punt in
> +         case of an overflow).  */
> +      step_var = build_int_cst (NULL_TREE, compare_step);
> +      gcc_assert (TREE_CODE (step_var) == INTEGER_CST);

See above, this would be unnecessary.
> +
> +      /* Compute (loop_bound - base) / compare_step.  */
> +      loop_count_var
> +        = int_const_binop (TRUNC_DIV_EXPR,
> +			   int_const_binop (MINUS_EXPR,
> +					    loop_bound_var,
> +					    compare_base),
> +			   step_var);
> +
> +      if (TREE_OVERFLOW_P (loop_count_var))
> +	  return;
> +
> +      HOST_WIDE_INT loop_count = tree_low_cst (loop_count_var, 0);

I thought you'd do the rest of the computations on trees too,
there are the risks of overflows or undefined behavior.

So if ((tree_int_cst_sgn (compare_step) == 1)
etc., integer_zerop (loop_count), and the only conversion from tree
to HWI would be after you convert the floating point tree to integer
when you assign it to probability.

	Jakub