On Thu, Feb 28, 2013 at 7:43 PM, Jakub Jelinek <ja...@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
