| Message ID | DB5PR08MB10306D92C15624041D357C5E837E0@DB5PR08MB1030.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR89437 | expand |
On February 21, 2019 4:55:57 PM GMT+01:00, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote: >Fix PR89437. Fix the sinatan-1.c testcase to not run without >a C99 target system. Use nextafterl for long double initialization. OK. >Fix an issue with sinl (atanl (sqrtl (LDBL_MAX)) returning 0.0 >instead of 1.0 by using x < sqrtl (LDBL_MAX) in match.pd. Wasn't that a intermediate problem with the mpfr exponent range limiting? Please check whether that's still needed. Richard. >OK for commit? > >ChangeLog: >2019-02-21 Wilco Dijkstra <wdijkstr@arm.com> > > gcc/ > * match.pd: Use lt in sin(atan(x)) and cos(atan(x)) simplifications. > > testsuite/ > * gcc.dg/sinatan-1.c: Fix testcase. >-- >diff --git a/gcc/match.pd b/gcc/match.pd >index >bccf4df05a2f94785446719b3097b3f912fafe96..c9af2e59441c4fe19e88d94c9d138ae35dfe673f >100644 >--- a/gcc/match.pd >+++ b/gcc/match.pd >@@ -4407,7 +4407,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > tree t_one = build_one_cst (type); > } > (if (SCALAR_FLOAT_TYPE_P (type)) >- (cond (le (abs @0) { t_cst; }) >+ (cond (lt (abs @0) { t_cst; }) > (rdiv @0 (sqrts (plus (mult @0 @0) { t_one; }))) > (copysigns { t_one; } @0)))))) > >@@ -4427,7 +4427,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > tree t_zero = build_zero_cst (type); > } > (if (SCALAR_FLOAT_TYPE_P (type)) >- (cond (le (abs @0) { t_cst; }) >+ (cond (lt (abs @0) { t_cst; }) > (rdiv { t_one; } (sqrts (plus (mult @0 @0) { t_one; }))) > (copysigns { t_zero; } @0)))))) > >diff --git a/gcc/testsuite/gcc.dg/sinatan-1.c >b/gcc/testsuite/gcc.dg/sinatan-1.c >index >6a3995ae07949a423e416592276171b6e6f8c816..cfbb771a018d3175c680dce88e0121e469716edc >100644 >--- a/gcc/testsuite/gcc.dg/sinatan-1.c >+++ b/gcc/testsuite/gcc.dg/sinatan-1.c >@@ -1,4 +1,4 @@ >-/* { dg-do run } */ >+/* { dg-do run { target c99_runtime } } */ > /* { dg-options "-Ofast" } */ > /* { dg-add-options ieee } */ > >@@ -62,7 +62,7 @@ main() > /* Get first x such that 1 + x*x will overflow */ > float fc = nextafterf (sqrtf (__FLT_MAX__ - 1), __FLT_MAX__); > double c = nextafter (sqrt (__DBL_MAX__ - 1), __DBL_MAX__); >- long double lc = nextafter (sqrtl (__LDBL_MAX__ - 1), >__LDBL_MAX__); >+ long double lc = nextafterl (sqrtl (__LDBL_MAX__ - 1), >__LDBL_MAX__); > > /* Force move from FPU to memory, otherwise comparison may > fail due to possible more accurate registers (see 387) */
I've just submitted a patch for this too :-P. Sorry about that. What is your nick in IRC, Wilco? On 02/21, Wilco Dijkstra wrote: > Fix PR89437. Fix the sinatan-1.c testcase to not run without > a C99 target system. Use nextafterl for long double initialization. > > Fix an issue with sinl (atanl (sqrtl (LDBL_MAX)) returning 0.0 > instead of 1.0 by using x < sqrtl (LDBL_MAX) in match.pd. > > OK for commit? > > ChangeLog: > 2019-02-21 Wilco Dijkstra <wdijkstr@arm.com> > > gcc/ > * match.pd: Use lt in sin(atan(x)) and cos(atan(x)) simplifications. > > testsuite/ > * gcc.dg/sinatan-1.c: Fix testcase. > -- > diff --git a/gcc/match.pd b/gcc/match.pd > index bccf4df05a2f94785446719b3097b3f912fafe96..c9af2e59441c4fe19e88d94c9d138ae35dfe673f 100644 > --- a/gcc/match.pd > +++ b/gcc/match.pd > @@ -4407,7 +4407,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > tree t_one = build_one_cst (type); > } > (if (SCALAR_FLOAT_TYPE_P (type)) > - (cond (le (abs @0) { t_cst; }) > + (cond (lt (abs @0) { t_cst; }) > (rdiv @0 (sqrts (plus (mult @0 @0) { t_one; }))) > (copysigns { t_one; } @0)))))) > > @@ -4427,7 +4427,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > tree t_zero = build_zero_cst (type); > } > (if (SCALAR_FLOAT_TYPE_P (type)) > - (cond (le (abs @0) { t_cst; }) > + (cond (lt (abs @0) { t_cst; }) > (rdiv { t_one; } (sqrts (plus (mult @0 @0) { t_one; }))) > (copysigns { t_zero; } @0)))))) > > diff --git a/gcc/testsuite/gcc.dg/sinatan-1.c b/gcc/testsuite/gcc.dg/sinatan-1.c > index 6a3995ae07949a423e416592276171b6e6f8c816..cfbb771a018d3175c680dce88e0121e469716edc 100644 > --- a/gcc/testsuite/gcc.dg/sinatan-1.c > +++ b/gcc/testsuite/gcc.dg/sinatan-1.c > @@ -1,4 +1,4 @@ > -/* { dg-do run } */ > +/* { dg-do run { target c99_runtime } } */ > /* { dg-options "-Ofast" } */ > /* { dg-add-options ieee } */ > > @@ -62,7 +62,7 @@ main() > /* Get first x such that 1 + x*x will overflow */ > float fc = nextafterf (sqrtf (__FLT_MAX__ - 1), __FLT_MAX__); > double c = nextafter (sqrt (__DBL_MAX__ - 1), __DBL_MAX__); > - long double lc = nextafter (sqrtl (__LDBL_MAX__ - 1), __LDBL_MAX__); > + long double lc = nextafterl (sqrtl (__LDBL_MAX__ - 1), __LDBL_MAX__); > > /* Force move from FPU to memory, otherwise comparison may > fail due to possible more accurate registers (see 387) */
Hi Richard, >>Fix an issue with sinl (atanl (sqrtl (LDBL_MAX)) returning 0.0 >>instead of 1.0 by using x < sqrtl (LDBL_MAX) in match.pd. > > Wasn't that a intermediate problem with the mpfr exponent range limiting? > Please check whether that's still needed. I tested it with trunk about an hour ago, and it included Jacub's patch. Are there other fixes outstanding which haven't been committed yet? Latest trunk also still gives an assertion failure in mpc with the gcc.dg/torture/builtin-math-5.c which started at the same time as the other mpc/mpfr releated issues: build/src/mpc/src/pow.c:631: MPC assertion failed: z_imag || mpfr_number_p (MPC_RE(u)) build/src/gcc/gcc/testsuite/gcc.dg/torture/builtin-math-5.c:95:3: internal compiler error: Aborted 0x6725ab crash_signal build/src/gcc/gcc/toplev.c:326 Wilco
On 2/21/19 8:55 AM, Wilco Dijkstra wrote: > Fix PR89437. Fix the sinatan-1.c testcase to not run without > a C99 target system. Use nextafterl for long double initialization. > > Fix an issue with sinl (atanl (sqrtl (LDBL_MAX)) returning 0.0 > instead of 1.0 by using x < sqrtl (LDBL_MAX) in match.pd. > > OK for commit? > > ChangeLog: > 2019-02-21 Wilco Dijkstra <wdijkstr@arm.com> > > gcc/ > * match.pd: Use lt in sin(atan(x)) and cos(atan(x)) simplifications. > > testsuite/ > * gcc.dg/sinatan-1.c: Fix testcase. OK. jeff
On Thu, Feb 21, 2019 at 6:09 PM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote: > > Hi Richard, > > >>Fix an issue with sinl (atanl (sqrtl (LDBL_MAX)) returning 0.0 > >>instead of 1.0 by using x < sqrtl (LDBL_MAX) in match.pd. > > > > Wasn't that a intermediate problem with the mpfr exponent range limiting? > > Please check whether that's still needed. > > I tested it with trunk about an hour ago, and it included Jacub's patch. > Are there other fixes outstanding which haven't been committed yet? Not that I know of. Did we root-cause the bogus folding to 0.0? Because I don't really understand why using < can "fix" this... > Latest trunk also still gives an assertion failure in mpc with the gcc.dg/torture/builtin-math-5.c > which started at the same time as the other mpc/mpfr releated issues: > > build/src/mpc/src/pow.c:631: MPC assertion failed: z_imag || mpfr_number_p (MPC_RE(u)) > build/src/gcc/gcc/testsuite/gcc.dg/torture/builtin-math-5.c:95:3: internal compiler error: Aborted > 0x6725ab crash_signal > build/src/gcc/gcc/toplev.c:326 Ick. Is there a PR about this? Richard. > Wilco
Hi Richard, >On Thu, Feb 21, 2019 at 6:09 PM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote: >> >> Hi Richard, >> >> >>Fix an issue with sinl (atanl (sqrtl (LDBL_MAX)) returning 0.0 >> >>instead of 1.0 by using x < sqrtl (LDBL_MAX) in match.pd. >> > >> > Wasn't that a intermediate problem with the mpfr exponent range limiting? >> > Please check whether that's still needed. >> >> I tested it with trunk about an hour ago, and it included Jacub's patch. >> Are there other fixes outstanding which haven't been committed yet? > > Not that I know of. Did we root-cause the bogus folding to 0.0? Because > I don't really understand why using < can "fix" this... Yes, the underlying issue is that build_sinatan_real returns the first value that does overflow when squared. Maybe that wasn't intended, but using less-than on the first value that does overflow works. With my patch (now committed) the test passes in all rounding modes. Like I mentioned, in the future this check could use a much smaller value based on the size of the mantissa - that's safer since you're not close to infinity. > Latest trunk also still gives an assertion failure in mpc with the gcc.dg/torture/builtin-math-5.c > which started at the same time as the other mpc/mpfr releated issues: > > build/src/mpc/src/pow.c:631: MPC assertion failed: z_imag || mpfr_number_p (MPC_RE(u)) > build/src/gcc/gcc/testsuite/gcc.dg/torture/builtin-math-5.c:95:3: internal compiler error: Aborted > 0x6725ab crash_signal > build/src/gcc/gcc/toplev.c:326 > > Ick. Is there a PR about this? This happens when using an old mpc (0.8.2). It's valid according to the configure check, however it works with the 1.0.3 version that download-prerequisites uses. Maybe we should increase the minimum mpc version in configure? Wilco
On Mon, Mar 4, 2019 at 2:39 PM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote: > > Hi Richard, > > >On Thu, Feb 21, 2019 at 6:09 PM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote: > >> > >> Hi Richard, > >> > >> >>Fix an issue with sinl (atanl (sqrtl (LDBL_MAX)) returning 0.0 > >> >>instead of 1.0 by using x < sqrtl (LDBL_MAX) in match.pd. > >> > > >> > Wasn't that a intermediate problem with the mpfr exponent range limiting? > >> > Please check whether that's still needed. > >> > >> I tested it with trunk about an hour ago, and it included Jacub's patch. > >> Are there other fixes outstanding which haven't been committed yet? > > > > Not that I know of. Did we root-cause the bogus folding to 0.0? Because > > I don't really understand why using < can "fix" this... > > Yes, the underlying issue is that build_sinatan_real returns the first value that does > overflow when squared. Maybe that wasn't intended, but using less-than on the first > value that does overflow works. With my patch (now committed) the test passes in > all rounding modes. > > Like I mentioned, in the future this check could use a much smaller value based on > the size of the mantissa - that's safer since you're not close to infinity. > > > Latest trunk also still gives an assertion failure in mpc with the gcc.dg/torture/builtin-math-5.c > > which started at the same time as the other mpc/mpfr releated issues: > > > > build/src/mpc/src/pow.c:631: MPC assertion failed: z_imag || mpfr_number_p (MPC_RE(u)) > > build/src/gcc/gcc/testsuite/gcc.dg/torture/builtin-math-5.c:95:3: internal compiler error: Aborted > > 0x6725ab crash_signal > > build/src/gcc/gcc/toplev.c:326 > > > > Ick. Is there a PR about this? > > This happens when using an old mpc (0.8.2). It's valid according to the configure check, > however it works with the 1.0.3 version that download-prerequisites uses. Maybe we should > increase the minimum mpc version in configure? I guess it might be enough to adjust the recommended version and notice caveats when using older ones in install.texi (IIRC we already do that to some extent). Richard. > Wilco
diff --git a/gcc/match.pd b/gcc/match.pd index bccf4df05a2f94785446719b3097b3f912fafe96..c9af2e59441c4fe19e88d94c9d138ae35dfe673f 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -4407,7 +4407,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) tree t_one = build_one_cst (type); } (if (SCALAR_FLOAT_TYPE_P (type)) - (cond (le (abs @0) { t_cst; }) + (cond (lt (abs @0) { t_cst; }) (rdiv @0 (sqrts (plus (mult @0 @0) { t_one; }))) (copysigns { t_one; } @0)))))) @@ -4427,7 +4427,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) tree t_zero = build_zero_cst (type); } (if (SCALAR_FLOAT_TYPE_P (type)) - (cond (le (abs @0) { t_cst; }) + (cond (lt (abs @0) { t_cst; }) (rdiv { t_one; } (sqrts (plus (mult @0 @0) { t_one; }))) (copysigns { t_zero; } @0)))))) diff --git a/gcc/testsuite/gcc.dg/sinatan-1.c b/gcc/testsuite/gcc.dg/sinatan-1.c index 6a3995ae07949a423e416592276171b6e6f8c816..cfbb771a018d3175c680dce88e0121e469716edc 100644 --- a/gcc/testsuite/gcc.dg/sinatan-1.c +++ b/gcc/testsuite/gcc.dg/sinatan-1.c @@ -1,4 +1,4 @@ -/* { dg-do run } */ +/* { dg-do run { target c99_runtime } } */ /* { dg-options "-Ofast" } */ /* { dg-add-options ieee } */ @@ -62,7 +62,7 @@ main() /* Get first x such that 1 + x*x will overflow */ float fc = nextafterf (sqrtf (__FLT_MAX__ - 1), __FLT_MAX__); double c = nextafter (sqrt (__DBL_MAX__ - 1), __DBL_MAX__); - long double lc = nextafter (sqrtl (__LDBL_MAX__ - 1), __LDBL_MAX__); + long double lc = nextafterl (sqrtl (__LDBL_MAX__ - 1), __LDBL_MAX__); /* Force move from FPU to memory, otherwise comparison may fail due to possible more accurate registers (see 387) */
Fix PR89437. Fix the sinatan-1.c testcase to not run without a C99 target system. Use nextafterl for long double initialization. Fix an issue with sinl (atanl (sqrtl (LDBL_MAX)) returning 0.0 instead of 1.0 by using x < sqrtl (LDBL_MAX) in match.pd. OK for commit? ChangeLog: 2019-02-21 Wilco Dijkstra <wdijkstr@arm.com> gcc/ * match.pd: Use lt in sin(atan(x)) and cos(atan(x)) simplifications. testsuite/ * gcc.dg/sinatan-1.c: Fix testcase. --