Fix PR56478

The following testcase ICEd because we were trying to negate
"-9223372036854775808", which results in UB, since it's LLONG_MAX + 1.
Then we got a SIGFPE on
-LLONG_MIN / -1
So fixed by doing the arithmetics on trees, and in case we got
an overflow, we bail out.

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.

Regtested/bootstrapped on x86_64-linux, ok for trunk?

2013-02-28  Marek Polacek  <polacek@redhat.com>

	PR tree-optimization/56478
	* predict.c (is_comparison_with_loop_invariant_p): Change the
	type of step to HOST_WIDE_INT.
	(predict_iv_comparison): Do the computation on trees, return
	in case of an overflow.

	* gcc.dg/torture/pr56478.c: New test.


	Marek

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

On Thu, Feb 28, 2013 at 7:43 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 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);

Don't use NULL_TREE built_int_cst - doing so hints at that you want to
use double_ints.  Generally doing computation with trees is expensive.
You want to avoid that at all cost.  Use double-ints (yeah, you have to
use the clunky divmod_with_overflow interface).

Richard.

>> +      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