Patchwork Fix PR56478

login
register
mail settings
Submitter Marek Polacek
Date Feb. 28, 2013, 6:27 p.m.
Message ID <20130228182748.GD15445@redhat.com>
Download mbox | patch
Permalink /patch/224131/
State New
Headers show

Comments

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

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