diff mbox series

[2/2] math: Add X_TLOSSf [BZ #28713]

Message ID 20211220231817.4051571-2-hjl.tools@gmail.com
State Superseded
Headers show
Series [1/2] s_sincosf.h: Change pio4 type to float [BZ #28713] | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

H.J. Lu Dec. 20, 2021, 11:18 p.m. UTC
Change X_TLOSS to the hexadecimal notation and add X_TLOSSf to define
the float verion of X_TLOSS to fix

FAIL: math/test-float-j0
FAIL: math/test-float-jn
FAIL: math/test-float-y0
FAIL: math/test-float-y1
FAIL: math/test-float-yn
FAIL: math/test-float32-j0
FAIL: math/test-float32-jn
FAIL: math/test-float32-y0
FAIL: math/test-float32-y1
FAIL: math/test-float32-yn

when compiling with GCC 12.
---
 math/math-svid-compat.h | 3 ++-
 math/w_j0f_compat.c     | 4 ++--
 math/w_j1f_compat.c     | 2 +-
 math/w_jnf_compat.c     | 4 ++--
 4 files changed, 7 insertions(+), 6 deletions(-)

Comments

Paul Zimmermann Dec. 21, 2021, 9:14 a.m. UTC | #1
Dear HJ,

> Date: Mon, 20 Dec 2021 15:18:17 -0800
> From: "H.J. Lu via Libc-alpha" <libc-alpha@sourceware.org>
> Cc: Joseph Myers <joseph@codesourcery.com>
> 
> Change X_TLOSS to the hexadecimal notation and add X_TLOSSf to define
> the float verion of X_TLOSS to fix
> 
> FAIL: math/test-float-j0
> FAIL: math/test-float-jn
> FAIL: math/test-float-y0
> FAIL: math/test-float-y1
> FAIL: math/test-float-yn
> FAIL: math/test-float32-j0
> FAIL: math/test-float32-jn
> FAIL: math/test-float32-y0
> FAIL: math/test-float32-y1
> FAIL: math/test-float32-yn
> 
> when compiling with GCC 12.

I confirm these tests fail with gcc 12, and pass with the patch below
(tested on x86_64 linux).

>  math/math-svid-compat.h | 3 ++-
>  math/w_j0f_compat.c     | 4 ++--
>  math/w_j1f_compat.c     | 2 +-
>  math/w_jnf_compat.c     | 4 ++--
>  4 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/math/math-svid-compat.h b/math/math-svid-compat.h
> index 5c18cb1b03..61d22ce461 100644
> --- a/math/math-svid-compat.h
> +++ b/math/math-svid-compat.h
> @@ -48,7 +48,8 @@ struct exception
>  extern int matherr (struct exception *__exc);
>  extern int __matherr (struct exception *__exc);
>  
> -#define X_TLOSS	1.41484755040568800000e+16
> +#define X_TLOSS		0x1.921fb54442d180000000p+53

why did you change the alignement?

> +#define X_TLOSSf	0x1.921fb6p+53

I would add a suffix 'f'

>  /* Types of exceptions in the `type' field.  */
>  #define DOMAIN		1
> diff --git a/math/w_j0f_compat.c b/math/w_j0f_compat.c
> index d375f3bfb0..7e8be846cb 100644
> --- a/math/w_j0f_compat.c
> +++ b/math/w_j0f_compat.c
> @@ -27,7 +27,7 @@
>  float
>  __j0f (float x)
>  {
> -  if (__builtin_expect (isgreater (fabsf (x), (float) X_TLOSS), 0)
> +  if (__builtin_expect (isgreater (fabsf (x), X_TLOSSf), 0)
>        && _LIB_VERSION != _IEEE_ && _LIB_VERSION != _POSIX_)
>      /* j0(|x|>X_TLOSS) */
>      return __kernel_standard_f (x, x, 134);
> @@ -42,7 +42,7 @@ float
>  __y0f (float x)
>  {
>    if (__builtin_expect (islessequal (x, 0.0f)
> -                        || isgreater (x, (float) X_TLOSS), 0)
> +                        || isgreater (x, X_TLOSSf), 0)
>        && _LIB_VERSION != _IEEE_)
>      {
>        if (x < 0.0f)
> diff --git a/math/w_j1f_compat.c b/math/w_j1f_compat.c
> index 81e56b771e..471dd93fdd 100644
> --- a/math/w_j1f_compat.c
> +++ b/math/w_j1f_compat.c
> @@ -42,7 +42,7 @@ float
>  __y1f (float x)
>  {
>    if (__builtin_expect (islessequal (x, 0.0f)
> -			|| isgreater (x, (float) X_TLOSS), 0)
> +			|| isgreater (x, X_TLOSSf), 0)
>        && _LIB_VERSION != _IEEE_)
>      {
>        if (x < 0.0f)
> diff --git a/math/w_jnf_compat.c b/math/w_jnf_compat.c
> index 296e631566..80af01385a 100644
> --- a/math/w_jnf_compat.c
> +++ b/math/w_jnf_compat.c
> @@ -27,7 +27,7 @@
>  float
>  __jnf (int n, float x)
>  {
> -  if (__builtin_expect (isgreater (fabsf (x), (float) X_TLOSS), 0)
> +  if (__builtin_expect (isgreater (fabsf (x), X_TLOSSf), 0)
>        && _LIB_VERSION != _IEEE_ && _LIB_VERSION != _POSIX_)
>      /* jn(n,|x|>X_TLOSS) */
>      return __kernel_standard_f (n, x, 138);
> @@ -42,7 +42,7 @@ float
>  __ynf (int n, float x)
>  {
>    if (__builtin_expect (islessequal (x, 0.0f)
> -			|| isgreater (x, (float) X_TLOSS), 0)
> +			|| isgreater (x, X_TLOSSf), 0)
>        && _LIB_VERSION != _IEEE_)
>      {
>        if (x < 0.0f)
> -- 
> 2.33.1

looks good to me, thanks!

Paul

PS: after this patch, and the previous one, there is only one remaining
failure with gcc12:
FAIL: elf/tst-env-setuid
Florian Weimer Dec. 21, 2021, 10:13 a.m. UTC | #2
* H. J. Lu via Libc-alpha:

> diff --git a/math/math-svid-compat.h b/math/math-svid-compat.h
> index 5c18cb1b03..61d22ce461 100644
> --- a/math/math-svid-compat.h
> +++ b/math/math-svid-compat.h
> @@ -48,7 +48,8 @@ struct exception
>  extern int matherr (struct exception *__exc);
>  extern int __matherr (struct exception *__exc);
>  
> -#define X_TLOSS	1.41484755040568800000e+16
> +#define X_TLOSS		0x1.921fb54442d180000000p+53
> +#define X_TLOSSf	0x1.921fb6p+53

Is it possible to use a preprocessor macro with token pasting to create
a correctly rounded flaot constant from X_TLOSS?

#define AS_FLOAT_CONSTANT_1(x) x##f
#define AS_FLOAT_CONSTANT(x) AS_FLOAT_CONSTANT_1(x)

I expect that GCC will not raise any exceptions when such a constant is
evaluated at run time.

Thanks,
Florian
Paul Zimmermann Dec. 21, 2021, 10:34 a.m. UTC | #3
Florian,

> Date: Tue, 21 Dec 2021 11:13:52 +0100
> From: Florian Weimer via Libc-alpha <libc-alpha@sourceware.org>
> Cc: Joseph Myers <joseph@codesourcery.com>
> 
> * H. J. Lu via Libc-alpha:
> 
> > diff --git a/math/math-svid-compat.h b/math/math-svid-compat.h
> > index 5c18cb1b03..61d22ce461 100644
> > --- a/math/math-svid-compat.h
> > +++ b/math/math-svid-compat.h
> > @@ -48,7 +48,8 @@ struct exception
> >  extern int matherr (struct exception *__exc);
> >  extern int __matherr (struct exception *__exc);
> >  
> > -#define X_TLOSS	1.41484755040568800000e+16
> > +#define X_TLOSS		0x1.921fb54442d180000000p+53
> > +#define X_TLOSSf	0x1.921fb6p+53
> 
> Is it possible to use a preprocessor macro with token pasting to create
> a correctly rounded flaot constant from X_TLOSS?
> 
> #define AS_FLOAT_CONSTANT_1(x) x##f
> #define AS_FLOAT_CONSTANT(x) AS_FLOAT_CONSTANT_1(x)

nice trick!

> I expect that GCC will not raise any exceptions when such a constant is
> evaluated at run time.

is there a chance that the constant is evaluated at run time? In such a case,
could it be that 0x1.921fb54442d180000000p+53f gets different values depending
on the rounding mode, in which case we would get back to the gcc12 failures.

Paul
H.J. Lu Dec. 21, 2021, 5:01 p.m. UTC | #4
On Tue, Dec 21, 2021 at 2:34 AM Paul Zimmermann
<Paul.Zimmermann@inria.fr> wrote:
>
>        Florian,
>
> > Date: Tue, 21 Dec 2021 11:13:52 +0100
> > From: Florian Weimer via Libc-alpha <libc-alpha@sourceware.org>
> > Cc: Joseph Myers <joseph@codesourcery.com>
> >
> > * H. J. Lu via Libc-alpha:
> >
> > > diff --git a/math/math-svid-compat.h b/math/math-svid-compat.h
> > > index 5c18cb1b03..61d22ce461 100644
> > > --- a/math/math-svid-compat.h
> > > +++ b/math/math-svid-compat.h
> > > @@ -48,7 +48,8 @@ struct exception
> > >  extern int matherr (struct exception *__exc);
> > >  extern int __matherr (struct exception *__exc);
> > >
> > > -#define X_TLOSS    1.41484755040568800000e+16
> > > +#define X_TLOSS            0x1.921fb54442d180000000p+53
> > > +#define X_TLOSSf   0x1.921fb6p+53
> >
> > Is it possible to use a preprocessor macro with token pasting to create
> > a correctly rounded flaot constant from X_TLOSS?
> >
> > #define AS_FLOAT_CONSTANT_1(x) x##f
> > #define AS_FLOAT_CONSTANT(x) AS_FLOAT_CONSTANT_1(x)
>
> nice trick!

Fixed.

The v2 patch is at

https://sourceware.org/pipermail/libc-alpha/2021-December/134490.html

> > I expect that GCC will not raise any exceptions when such a constant is
> > evaluated at run time.
>
> is there a chance that the constant is evaluated at run time? In such a case,
> could it be that 0x1.921fb54442d180000000p+53f gets different values depending
> on the rounding mode, in which case we would get back to the gcc12 failures.
>
> Paul

Thanks.
Paul Zimmermann Dec. 22, 2021, 9:11 a.m. UTC | #5
Dear HJ,

> The v2 patch is at
> 
> https://sourceware.org/pipermail/libc-alpha/2021-December/134490.html

at first glance this looks ok to me, but I did not get an answer to that:

> > is there a chance that the constant is evaluated at run time? In such a case,
> > could it be that 0x1.921fb54442d180000000p+53f gets different values depending
> > on the rounding mode, in which case we would get back to the gcc12 failures.

Paul
H.J. Lu Dec. 22, 2021, 1:21 p.m. UTC | #6
On Wed, Dec 22, 2021 at 1:11 AM Paul Zimmermann
<Paul.Zimmermann@inria.fr> wrote:
>
>        Dear HJ,
>
> > The v2 patch is at
> >
> > https://sourceware.org/pipermail/libc-alpha/2021-December/134490.html
>
> at first glance this looks ok to me, but I did not get an answer to that:
>
> > > is there a chance that the constant is evaluated at run time? In such a case,
> > > could it be that 0x1.921fb54442d180000000p+53f gets different values depending
> > > on the rounding mode, in which case we would get back to the gcc12 failures.
>
> Paul

The C standard says that "An unsuffixed floating constant has type
double. If suffixed by
the letter f or F, it has type float."
Paul Zimmermann Dec. 22, 2021, 1:35 p.m. UTC | #7
Hi HJ,

> > > The v2 patch is at
> > >
> > > https://sourceware.org/pipermail/libc-alpha/2021-December/134490.html
> >
> > at first glance this looks ok to me, but I did not get an answer to that:
> >
> > > > is there a chance that the constant is evaluated at run time? In such a case,
> > > > could it be that 0x1.921fb54442d180000000p+53f gets different values depending
> > > > on the rounding mode, in which case we would get back to the gcc12 failures.
> >
> > Paul
> 
> The C standard says that "An unsuffixed floating constant has type
> double. If suffixed by
> the letter f or F, it has type float."

sorry this was not my question. My concern is whether
AS_FLOAT_CONSTANT(0x1.921fb54442d180000000p+53) is
evaluated at compile-time or run-time.

If at compile-time, the C standard says that it should be evaluated with
the default rounding mode (nearest ties to even), thus we always get
0x1.921fb6p1.

If at run-time, we could get different values depending on the current
rounding mode, and the bug you are trying to fix could come up again.

Can someone clarify this?

Paul
H.J. Lu Dec. 22, 2021, 1:50 p.m. UTC | #8
On Wed, Dec 22, 2021 at 5:35 AM Paul Zimmermann
<Paul.Zimmermann@inria.fr> wrote:
>
>        Hi HJ,
>
> > > > The v2 patch is at
> > > >
> > > > https://sourceware.org/pipermail/libc-alpha/2021-December/134490.html
> > >
> > > at first glance this looks ok to me, but I did not get an answer to that:
> > >
> > > > > is there a chance that the constant is evaluated at run time? In such a case,
> > > > > could it be that 0x1.921fb54442d180000000p+53f gets different values depending
> > > > > on the rounding mode, in which case we would get back to the gcc12 failures.
> > >
> > > Paul
> >
> > The C standard says that "An unsuffixed floating constant has type
> > double. If suffixed by
> > the letter f or F, it has type float."
>
> sorry this was not my question. My concern is whether
> AS_FLOAT_CONSTANT(0x1.921fb54442d180000000p+53) is
> evaluated at compile-time or run-time.
>
> If at compile-time, the C standard says that it should be evaluated with
> the default rounding mode (nearest ties to even), thus we always get
> 0x1.921fb6p1.

Since a floating constant is handled at compile-time by C frontend:

#0  real_from_string3 (r=0x7fffffffc4b0,
    s=0x7fffffffc420 "0x1.921fb54442d180000000p+53", fmt=...)
    at /export/gnu/import/git/gitlab/x86-gcc/gcc/real.c:2173
#1  0x0000000000ba9e78 in interpret_float (token=0x3a1add0, flags=530,
    suffix=0x0, overflow=0x7fffffffc6c4)
    at /export/gnu/import/git/gitlab/x86-gcc/gcc/c-family/c-lex.c:1035
#2  0x0000000000ba8cf3 in c_lex_with_flags (value=0x7ffff7fc7be0,
    loc=0x7ffff7fc7bdc, cpp_flags=0x7ffff7fc7be8 "\001", lex_flags=2)
    at /export/gnu/import/git/gitlab/x86-gcc/gcc/c-family/c-lex.c:517
#3  0x0000000000aec392 in c_lex_one_token (parser=0x7ffff7fc7bd0,
    token=0x7ffff7fc7bd8, raw=false)
    at /export/gnu/import/git/gitlab/x86-gcc/gcc/c/c-parser.c:279
#4  0x0000000000aec8cd in c_parser_peek_token (parser=0x7ffff7fc7bd0)
    at /export/gnu/import/git/gitlab/x86-gcc/gcc/c/c-parser.c:483
#5  0x0000000000aebf02 in c_parser_next_token_is (parser=0x7ffff7fc7bd0,
    type=CPP_CLOSE_PAREN)
    at /export/gnu/import/git/gitlab/x86-gcc/gcc/c/c-parser.h:167
#6  0x0000000000b0646d in c_parser_postfix_expression_after_primary (
    parser=0x7ffff7fc7bd0, expr_loc=20642, expr=...)
    at /export/gnu/import/git/gitlab/x86-gcc/gcc/c/c-parser.c:10529

> If at run-time, we could get different values depending on the current
> rounding mode, and the bug you are trying to fix could come up again.
>
> Can someone clarify this?
>
> Paul
Florian Weimer Dec. 22, 2021, 2:03 p.m. UTC | #9
* H. J. Lu:

> On Wed, Dec 22, 2021 at 5:35 AM Paul Zimmermann
> <Paul.Zimmermann@inria.fr> wrote:
>>
>>        Hi HJ,
>>
>> > > > The v2 patch is at
>> > > >
>> > > > https://sourceware.org/pipermail/libc-alpha/2021-December/134490.html
>> > >
>> > > at first glance this looks ok to me, but I did not get an answer to that:
>> > >
>> > > > > is there a chance that the constant is evaluated at run time? In such a case,
>> > > > > could it be that 0x1.921fb54442d180000000p+53f gets different values depending
>> > > > > on the rounding mode, in which case we would get back to the gcc12 failures.
>> > >
>> > > Paul
>> >
>> > The C standard says that "An unsuffixed floating constant has type
>> > double. If suffixed by
>> > the letter f or F, it has type float."
>>
>> sorry this was not my question. My concern is whether
>> AS_FLOAT_CONSTANT(0x1.921fb54442d180000000p+53) is
>> evaluated at compile-time or run-time.
>>
>> If at compile-time, the C standard says that it should be evaluated with
>> the default rounding mode (nearest ties to even), thus we always get
>> 0x1.921fb6p1.
>
> Since a floating constant is handled at compile-time by C frontend:

But maybe this is a bug?  It's a bit counter-intuitive that

  0x1.921fb54442d180000000p+53f

and

  (float) 0x1.921fb54442d180000000p+53

give different results.

Thanks,
Florian
H.J. Lu Dec. 22, 2021, 2:30 p.m. UTC | #10
On Wed, Dec 22, 2021 at 6:04 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Wed, Dec 22, 2021 at 5:35 AM Paul Zimmermann
> > <Paul.Zimmermann@inria.fr> wrote:
> >>
> >>        Hi HJ,
> >>
> >> > > > The v2 patch is at
> >> > > >
> >> > > > https://sourceware.org/pipermail/libc-alpha/2021-December/134490.html
> >> > >
> >> > > at first glance this looks ok to me, but I did not get an answer to that:
> >> > >
> >> > > > > is there a chance that the constant is evaluated at run time? In such a case,
> >> > > > > could it be that 0x1.921fb54442d180000000p+53f gets different values depending
> >> > > > > on the rounding mode, in which case we would get back to the gcc12 failures.
> >> > >
> >> > > Paul
> >> >
> >> > The C standard says that "An unsuffixed floating constant has type
> >> > double. If suffixed by
> >> > the letter f or F, it has type float."
> >>
> >> sorry this was not my question. My concern is whether
> >> AS_FLOAT_CONSTANT(0x1.921fb54442d180000000p+53) is
> >> evaluated at compile-time or run-time.
> >>
> >> If at compile-time, the C standard says that it should be evaluated with
> >> the default rounding mode (nearest ties to even), thus we always get
> >> 0x1.921fb6p1.
> >
> > Since a floating constant is handled at compile-time by C frontend:
>
> But maybe this is a bug?  It's a bit counter-intuitive that
>
>   0x1.921fb54442d180000000p+53f
>
> and
>
>   (float) 0x1.921fb54442d180000000p+53
>
> give different results.

It is the same as

static float f = 0x1.921fb54442d180000000p+53f;
Joseph Myers Dec. 30, 2021, 9:45 p.m. UTC | #11
On Wed, 22 Dec 2021, Paul Zimmermann wrote:

>        Dear HJ,
> 
> > The v2 patch is at
> > 
> > https://sourceware.org/pipermail/libc-alpha/2021-December/134490.html
> 
> at first glance this looks ok to me, but I did not get an answer to that:
> 
> > > is there a chance that the constant is evaluated at run time? In such a case,
> > > could it be that 0x1.921fb54442d180000000p+53f gets different values depending
> > > on the rounding mode, in which case we would get back to the gcc12 failures.

Floating constants are evaluated in the translation-time rounding mode, 
which is round-to-nearest unless the FENV_ROUND pragma is used.  They are 
never affected by the dynamic rounding mode.

Excess precision applies to floating constants (so when a floating 
constant is of a type with excess precision, it's evaluated - using the 
translation-time rounding mode - to a wider evaluation format).  
Depending on how the constant gets used, it's possible a narrowing to the 
range and precision of the type could occur at run time using the dynamic 
rounding mode.  This isn't relevant to glibc, given that we don't build 
with -fexcess-precision=standard and don't currently support _Float16 
functions (the case where excess precision is relevant without 
-fexcess-precision=standard).  Even with -fexcess precision=standard, it 
would only be relevant in cases where a runtime narrowing occurs (constant 
used for assignment / initialization / argument passing / return).
diff mbox series

Patch

diff --git a/math/math-svid-compat.h b/math/math-svid-compat.h
index 5c18cb1b03..61d22ce461 100644
--- a/math/math-svid-compat.h
+++ b/math/math-svid-compat.h
@@ -48,7 +48,8 @@  struct exception
 extern int matherr (struct exception *__exc);
 extern int __matherr (struct exception *__exc);
 
-#define X_TLOSS	1.41484755040568800000e+16
+#define X_TLOSS		0x1.921fb54442d180000000p+53
+#define X_TLOSSf	0x1.921fb6p+53
 
 /* Types of exceptions in the `type' field.  */
 #define DOMAIN		1
diff --git a/math/w_j0f_compat.c b/math/w_j0f_compat.c
index d375f3bfb0..7e8be846cb 100644
--- a/math/w_j0f_compat.c
+++ b/math/w_j0f_compat.c
@@ -27,7 +27,7 @@ 
 float
 __j0f (float x)
 {
-  if (__builtin_expect (isgreater (fabsf (x), (float) X_TLOSS), 0)
+  if (__builtin_expect (isgreater (fabsf (x), X_TLOSSf), 0)
       && _LIB_VERSION != _IEEE_ && _LIB_VERSION != _POSIX_)
     /* j0(|x|>X_TLOSS) */
     return __kernel_standard_f (x, x, 134);
@@ -42,7 +42,7 @@  float
 __y0f (float x)
 {
   if (__builtin_expect (islessequal (x, 0.0f)
-                        || isgreater (x, (float) X_TLOSS), 0)
+                        || isgreater (x, X_TLOSSf), 0)
       && _LIB_VERSION != _IEEE_)
     {
       if (x < 0.0f)
diff --git a/math/w_j1f_compat.c b/math/w_j1f_compat.c
index 81e56b771e..471dd93fdd 100644
--- a/math/w_j1f_compat.c
+++ b/math/w_j1f_compat.c
@@ -42,7 +42,7 @@  float
 __y1f (float x)
 {
   if (__builtin_expect (islessequal (x, 0.0f)
-			|| isgreater (x, (float) X_TLOSS), 0)
+			|| isgreater (x, X_TLOSSf), 0)
       && _LIB_VERSION != _IEEE_)
     {
       if (x < 0.0f)
diff --git a/math/w_jnf_compat.c b/math/w_jnf_compat.c
index 296e631566..80af01385a 100644
--- a/math/w_jnf_compat.c
+++ b/math/w_jnf_compat.c
@@ -27,7 +27,7 @@ 
 float
 __jnf (int n, float x)
 {
-  if (__builtin_expect (isgreater (fabsf (x), (float) X_TLOSS), 0)
+  if (__builtin_expect (isgreater (fabsf (x), X_TLOSSf), 0)
       && _LIB_VERSION != _IEEE_ && _LIB_VERSION != _POSIX_)
     /* jn(n,|x|>X_TLOSS) */
     return __kernel_standard_f (n, x, 138);
@@ -42,7 +42,7 @@  float
 __ynf (int n, float x)
 {
   if (__builtin_expect (islessequal (x, 0.0f)
-			|| isgreater (x, (float) X_TLOSS), 0)
+			|| isgreater (x, X_TLOSSf), 0)
       && _LIB_VERSION != _IEEE_)
     {
       if (x < 0.0f)