| Message ID | 51833D81.9050402@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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); >
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
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);
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.