Fix PR56478

Submitted by Marek Polacek on Feb. 28, 2013, 6:27 p.m.

Details

Message ID 20130228182748.GD15445@redhat.com
State New
Headers show

Commit Message

Marek Polacek Feb. 28, 2013, 6:27 p.m.
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

Comments

Jakub Jelinek Feb. 28, 2013, 6:43 p.m.
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
Richard Guenther March 1, 2013, 10:10 a.m.
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

Patch hide | download patch | download mbox

--- gcc/predict.c.mp	2013-02-28 17:26:47.950247877 +0100
+++ gcc/predict.c	2013-02-28 17:26:56.855275792 +0100
@@ -1034,7 +1034,7 @@  is_comparison_with_loop_invariant_p (gim
   tree op0, op1, bound, base;
   affine_iv iv0, iv1;
   enum tree_code code;
-  int step;
+  HOST_WIDE_INT step;
 
   code = gimple_cond_code (stmt);
   *loop_invariant = NULL;
@@ -1224,11 +1224,29 @@  predict_iv_comparison (struct loop *loop
       && host_integerp (compare_base, 0))
     {
       int probability;
+      tree step_var, loop_count_var;
       HOST_WIDE_INT compare_count;
       HOST_WIDE_INT loop_bound = tree_low_cst (loop_bound_var, 0);
       HOST_WIDE_INT compare_bound = tree_low_cst (compare_var, 0);
       HOST_WIDE_INT base = tree_low_cst (compare_base, 0);
-      HOST_WIDE_INT loop_count = (loop_bound - base) / compare_step;
+
+      /* 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);
+
+      /* 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);
 
       if ((compare_step > 0)
           ^ (compare_code == LT_EXPR || compare_code == LE_EXPR))
--- gcc/testsuite/gcc.dg/torture/pr56478.c.mp	2013-02-28 17:32:26.430308968 +0100
+++ gcc/testsuite/gcc.dg/torture/pr56478.c	2013-02-28 18:15:41.641379179 +0100
@@ -0,0 +1,12 @@ 
+/* PR tree-optimization/56478 */
+/* { dg-do compile } */
+
+int a;
+
+void
+foo ()
+{
+  int b;
+  for (b = 0;; b++)
+    a = 0 < -__LONG_LONG_MAX__ - 1 - b ? : 0;
+}