Patchwork [RFA] Fix tree-optimization/57144

login
register
mail settings
Submitter Jeff Law
Date May 3, 2013, 4:30 a.m.
Message ID <51833D81.9050402@redhat.com>
Download mbox | patch
Permalink /patch/241145/
State New
Headers show

Comments

Jeff Law - May 3, 2013, 4:30 a.m.
When we have

x = (T) y;
if (x > CONST)
   true arm
else
   false arm

Assume CONST is larger than what can be represented in T.  If we use 
fold_convert, any bits not not in T will be dropped.  So if CONST is say 
0x100000000 and T is a 32 bit type, the returned constant will be 0x0. 
So we change the code to

x = (T) y;
if (y > 0)
   true arm
else
   false arm

If y happens to have a value such as "1" we end up taking the true arm 
when we should have taken the false arm.

The astute reader will note that VRP should have simply eliminated the 
condition because it can never be true, at least not in the testcase 
which is derived from real code in mpfr.  Probably we're too 
conservative when propagating a [0..INF(OV)] through a widening conversion.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu, OK for 
trunk?

Note this fixes the mpfr-2.x testsuite failures on x86_64.  While I 
believe this fix will address 57144 (ia64), I haven't directly confirmed 
that's true.
commit baf070f82c7c6f19fd0a9e72514cc0ce03920c29
Author: Jeff Law <law@redhat.com>
Date:   Thu May 2 22:19:37 2013 -0600

    	PR tree-optimization/57411
    	* tree-vrp.c (simplify_cond_using_ranges): Verify the constant
    	operand of the condition will bit into the new type when eliminating
    	a cast feeding a condition.
    
    	PR tree-optimization/57411
    	* gcc.c-torture/execute/pr57144.c: New test.
Jakub Jelinek - May 3, 2013, 5:56 a.m.
On Thu, May 02, 2013 at 10:30:57PM -0600, Jeff Law wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr57144.c
> @@ -0,0 +1,15 @@
> +__attribute__ ((noinline))
> +foo(int a)
> +{
> +  int z = a > 0 ? a : -a;
> +  long x = z;
> +  if (x > 0x100000000)
> +    abort ();
> +  else
> +    exit (0);
> +}
> +
> +main()
> +{
> +  foo (1);
> +}

Just commenting on the testcase, could you please:
1) add return types (void to foo and int to main)
2) use long long instead of long, otherwise the testcase doesn't fail
   without your patch with -m32
3) I'd add LL suffix to the constant too
4) either prototype extern void abort (void); extern void exit (int);
   or use __builtin_{abort,exit}?

For 1) and 4) we usually add at least C89 testcases now, unless strictly
necessary (testing K&R stuff).  I believe with long long the testcase should
be fine even on say AVR, because C99 says that long long must be at least
64-bit.

	Jakub
Richard Guenther - May 3, 2013, 8:10 a.m.
On Fri, May 3, 2013 at 6:30 AM, Jeff Law <law@redhat.com> wrote:
>
> When we have
>
> x = (T) y;
> if (x > CONST)
>   true arm
> else
>   false arm
>
> Assume CONST is larger than what can be represented in T.  If we use
> fold_convert, any bits not not in T will be dropped.  So if CONST is say
> 0x100000000 and T is a 32 bit type, the returned constant will be 0x0. So we
> change the code to
>
> x = (T) y;
> if (y > 0)
>   true arm
> else
>   false arm
>
> If y happens to have a value such as "1" we end up taking the true arm when
> we should have taken the false arm.
>
> The astute reader will note that VRP should have simply eliminated the
> condition because it can never be true, at least not in the testcase which
> is derived from real code in mpfr.  Probably we're too conservative when
> propagating a [0..INF(OV)] through a widening conversion.
>
> Bootstrapped and regression tested on x86_64-unknown-linux-gnu, OK for
> trunk?

Ok with the testcase changes suggested by Jakub.

Thanks,
Richard.

> Note this fixes the mpfr-2.x testsuite failures on x86_64.  While I believe
> this fix will address 57144 (ia64), I haven't directly confirmed that's
> true.
>
>
>
>
>
>
> commit baf070f82c7c6f19fd0a9e72514cc0ce03920c29
> Author: Jeff Law <law@redhat.com>
> Date:   Thu May 2 22:19:37 2013 -0600
>
>         PR tree-optimization/57411
>         * tree-vrp.c (simplify_cond_using_ranges): Verify the constant
>         operand of the condition will bit into the new type when eliminating
>         a cast feeding a condition.
>
>         PR tree-optimization/57411
>         * gcc.c-torture/execute/pr57144.c: New test.
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 9e3d783..f245097 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,10 @@
> +2013-05-02  Jeff Law  <law@redhat.com>
> +
> +       PR tree-optimization/57411
> +       * tree-vrp.c (simplify_cond_using_ranges): Verify the constant
> +       operand of the condition will bit into the new type when eliminating
> +       a cast feeding a condition.
> +
>  2013-05-02  Richard Biener  <rguenther@suse.de>
>
>         * tree-eh.c (cleanup_empty_eh_merge_phis): Remove rename_virts
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index c8dc189..be30c51 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2013-05-02  Jeff Law  <law@redhat.com>
> +
> +       PR tree-optimization/57411
> +       * gcc.c-torture/execute/pr57144.c: New test.
> +
>  2013-05-02  Tobias Burnus  <burnus@net-b.de>
>
>         PR fortran/57142
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr57144.c
> b/gcc/testsuite/gcc.c-torture/execute/pr57144.c
> new file mode 100644
> index 0000000..e96c749
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr57144.c
> @@ -0,0 +1,15 @@
> +__attribute__ ((noinline))
> +foo(int a)
> +{
> +  int z = a > 0 ? a : -a;
> +  long x = z;
> +  if (x > 0x100000000)
> +    abort ();
> +  else
> +    exit (0);
> +}
> +
> +main()
> +{
> +  foo (1);
> +}
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index 6ed353f..b5de683 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -8668,7 +8668,8 @@ simplify_cond_using_ranges (gimple stmt)
>           if (range_int_cst_p (vr)
>               && range_fits_type_p (vr,
>                                     TYPE_PRECISION (TREE_TYPE (op0)),
> -                                   TYPE_UNSIGNED (TREE_TYPE (op0))))
> +                                   TYPE_UNSIGNED (TREE_TYPE (op0)))
> +             && int_fits_type_p (op1, TREE_TYPE (innerop)))
>             {
>               tree newconst = fold_convert (TREE_TYPE (innerop), op1);
>               gimple_cond_set_lhs (stmt, innerop);
>
Jeff Law - May 3, 2013, 3:28 p.m.
On 05/02/2013 11:56 PM, Jakub Jelinek wrote:
> On Thu, May 02, 2013 at 10:30:57PM -0600, Jeff Law wrote:
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.c-torture/execute/pr57144.c
>> @@ -0,0 +1,15 @@
>> +__attribute__ ((noinline))
>> +foo(int a)
>> +{
>> +  int z = a > 0 ? a : -a;
>> +  long x = z;
>> +  if (x > 0x100000000)
>> +    abort ();
>> +  else
>> +    exit (0);
>> +}
>> +
>> +main()
>> +{
>> +  foo (1);
>> +}
>
> Just commenting on the testcase, could you please:
> 1) add return types (void to foo and int to main)
> 2) use long long instead of long, otherwise the testcase doesn't fail
>     without your patch with -m32
> 3) I'd add LL suffix to the constant too
> 4) either prototype extern void abort (void); extern void exit (int);
>     or use __builtin_{abort,exit}?
Done.  I'll keep this in mind for future tests.

>
> For 1) and 4) we usually add at least C89 testcases now, unless strictly
> necessary (testing K&R stuff).  I believe with long long the testcase should
> be fine even on say AVR, because C99 says that long long must be at least
> 64-bit.
Makes sense; my how things change when you go away for a while! ;-)

jeff

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 9e3d783..f245097 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@ 
+2013-05-02  Jeff Law  <law@redhat.com>
+
+	PR tree-optimization/57411
+	* tree-vrp.c (simplify_cond_using_ranges): Verify the constant
+	operand of the condition will bit into the new type when eliminating
+	a cast feeding a condition.
+
 2013-05-02  Richard Biener  <rguenther@suse.de>
 
 	* tree-eh.c (cleanup_empty_eh_merge_phis): Remove rename_virts
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index c8dc189..be30c51 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2013-05-02  Jeff Law  <law@redhat.com>
+
+	PR tree-optimization/57411
+	* gcc.c-torture/execute/pr57144.c: New test.
+
 2013-05-02  Tobias Burnus  <burnus@net-b.de>
 
 	PR fortran/57142
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr57144.c b/gcc/testsuite/gcc.c-torture/execute/pr57144.c
new file mode 100644
index 0000000..e96c749
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr57144.c
@@ -0,0 +1,15 @@ 
+__attribute__ ((noinline))
+foo(int a)
+{
+  int z = a > 0 ? a : -a;
+  long x = z;
+  if (x > 0x100000000)
+    abort ();
+  else
+    exit (0);
+}
+
+main()
+{
+  foo (1);
+}
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 6ed353f..b5de683 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -8668,7 +8668,8 @@  simplify_cond_using_ranges (gimple stmt)
 	  if (range_int_cst_p (vr)
 	      && range_fits_type_p (vr,
 				    TYPE_PRECISION (TREE_TYPE (op0)),
-				    TYPE_UNSIGNED (TREE_TYPE (op0))))
+				    TYPE_UNSIGNED (TREE_TYPE (op0)))
+	      && int_fits_type_p (op1, TREE_TYPE (innerop)))
 	    {
 	      tree newconst = fold_convert (TREE_TYPE (innerop), op1);
 	      gimple_cond_set_lhs (stmt, innerop);