[PATCH] Fix PR89437

[PATCH] Fix PR89437

[Wilco Dijkstra]

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)  */

Re: [PATCH] Fix PR89437

[Richard Biener]

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)  */

Re: [PATCH] Fix PR89437

[Giuliano Belinassi]

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)  */

Re: [PATCH] Fix PR89437

[Wilco Dijkstra]

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

Re: [PATCH] Fix PR89437

[Jeff Law]

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

Re: [PATCH] Fix PR89437

[Richard Biener]

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

Re: [PATCH] Fix PR89437

[Wilco Dijkstra]

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

Re: [PATCH] Fix PR89437

[Richard Biener]

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