Re: [PATCH] Fix PR56478

[Richard Biener <richard dot guenther at gmail dot com>]

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