diff mbox series

Remap __GLIBC_FLT_EVAL_METHOD to 0 if __FLT_EVAL_METHOD__ is -1

Message ID 20230217022646.99959-1-kito.cheng@sifive.com
State New
Headers
Series Remap __GLIBC_FLT_EVAL_METHOD to 0 if __FLT_EVAL_METHOD__ is -1 |

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
redhat-pt-bot/TryBot-still_applies warning Patch no longer applies to master

Commit Message

Kito Cheng Feb. 17, 2023, 2:26 a.m. UTC 
  ---
We were intending to update RISC-V's setting only, but after discussion
with Wilco Dijkstra, we decide to change the generic one instead of RISC-V only
since it also fix inefficient issue for float_t and double_t.
---

__GLIBC_FLT_EVAL_METHOD will effect the definition of float_t and
double_t, currently we'll set __GLIBC_FLT_EVAL_METHOD to 2 when
__FLT_EVAL_METHOD__ is -1, that means we'll define float_t and double_t
to long double.

However some target isn't natively (HW) support long double like AArch64 and
RISC-V, they defined long double as 128-bits IEEE 754 floating point type.

That means setting __GLIBC_FLT_EVAL_METHOD to 2 will cause very inefficient
code gen for those target who didn't provide native support for long
double, and that's violate the spirit float_t and double_t - most efficient
types at least as wide as float and double.

So this patch propose to remap __GLIBC_FLT_EVAL_METHOD to 0 rather than
2 when __FLT_EVAL_METHOD__ is -1, which means we'll use float/double
rather than long double for float_t and double_t.

Note: __FLT_EVAL_METHOD__ == -1 means the precision is indeterminable,
      which means compiler might using indeterminable precision during
      optimization/code gen, clang will set this value to -1 when fast
      math is enabled.

Note: Default definition float_t and double_t in current glibc:
      |  __GLIBC_FLT_EVAL_METHOD | float_t     | double_t
      |               0 or 16    | float       | double
      |               1          | double      | doulbe
      |               2          | long double | long double
      More complete list see math/math.h

Note: RISC-V has defined ISA extension to support 128-bits IEEE 754
      floating point operations, but only rare RISC-V core will implement that.

Related link:

[1] LLVM issue (__FLT_EVAL_METHOD__ is set to -1 with Ofast. #60781):
    https://github.com/llvm/llvm-project/issues/60781
[2] Last version of this patch: https://sourceware.org/pipermail/libc-alpha/2023-February/145622.html

Ref:

[1] Definition of FLT_EVAL_METHOD from C99 spec:
C99 Spec draft: (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)

Except for assignment and cast (which remove all extra range and precision), the values
of operations with floating operands and values subject to the usual arithmetic
conversions and of floating constants are evaluated to a format whose range and precision
may be greater than required by the type. The use of evaluation formats is characterized
by the implementation-defined value of FLT_EVAL_METHOD:
19)

  -1 indeterminable;
  0 evaluate all operations and constants just to the range and precision of the
    type;
  1 evaluate operations and constants of type float and double to the
    range and precision of the double type, evaluate long double
    operations and constants to the range and precision of the long double
    type;
  2 evaluate all operations and constants to the range and precision of the
    long double type.

All other negative values for FLT_EVAL_METHOD characterize implementation-defined
behavior.

[2] Definition of float_t and double_t in C99 spec:

C99 Spec draft: (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)

7.12

...

The types
float_t
double_t
are floating types at least as wide as float and double, respectively, and such that
double_t is at least as wide as float_t. If FLT_EVAL_METHOD equals 0,
float_t and double_t are float and double, respectively; if
FLT_EVAL_METHOD equals 1, they are both double; if FLT_EVAL_METHOD equals
2, they are both long double; and for other values of FLT_EVAL_METHOD, they are
otherwise implementation-defined.199)

199) The types float_t and double_t are intended to be the implementation’s most efficient types at
least as wide as float and double, respectively. For FLT_EVAL_METHOD equal 0, 1, or 2, the
type float_t is the narrowest type used by the implementation to
evaluate floating expressions.
---
 bits/flt-eval-method.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Palmer Dabbelt Feb. 18, 2023, 6:29 p.m. UTC | #1 
On Thu, 16 Feb 2023 18:26:46 PST (-0800), kito.cheng@sifive.com wrote:
> ---
> We were intending to update RISC-V's setting only, but after discussion
> with Wilco Dijkstra, we decide to change the generic one instead of RISC-V only
> since it also fix inefficient issue for float_t and double_t.
> ---
>
> __GLIBC_FLT_EVAL_METHOD will effect the definition of float_t and
> double_t, currently we'll set __GLIBC_FLT_EVAL_METHOD to 2 when
> __FLT_EVAL_METHOD__ is -1, that means we'll define float_t and double_t
> to long double.
>
> However some target isn't natively (HW) support long double like AArch64 and
> RISC-V, they defined long double as 128-bits IEEE 754 floating point type.
>
> That means setting __GLIBC_FLT_EVAL_METHOD to 2 will cause very inefficient
> code gen for those target who didn't provide native support for long
> double, and that's violate the spirit float_t and double_t - most efficient
> types at least as wide as float and double.
>
> So this patch propose to remap __GLIBC_FLT_EVAL_METHOD to 0 rather than
> 2 when __FLT_EVAL_METHOD__ is -1, which means we'll use float/double
> rather than long double for float_t and double_t.
>
> Note: __FLT_EVAL_METHOD__ == -1 means the precision is indeterminable,
>       which means compiler might using indeterminable precision during
>       optimization/code gen, clang will set this value to -1 when fast
>       math is enabled.
>
> Note: Default definition float_t and double_t in current glibc:
>       |  __GLIBC_FLT_EVAL_METHOD | float_t     | double_t
>       |               0 or 16    | float       | double
>       |               1          | double      | doulbe
>       |               2          | long double | long double
>       More complete list see math/math.h
>
> Note: RISC-V has defined ISA extension to support 128-bits IEEE 754
>       floating point operations, but only rare RISC-V core will implement that.
>
> Related link:
>
> [1] LLVM issue (__FLT_EVAL_METHOD__ is set to -1 with Ofast. #60781):
>     https://github.com/llvm/llvm-project/issues/60781
> [2] Last version of this patch: https://sourceware.org/pipermail/libc-alpha/2023-February/145622.html
>
> Ref:
>
> [1] Definition of FLT_EVAL_METHOD from C99 spec:
> C99 Spec draft: (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)
>
> Except for assignment and cast (which remove all extra range and precision), the values
> of operations with floating operands and values subject to the usual arithmetic
> conversions and of floating constants are evaluated to a format whose range and precision
> may be greater than required by the type. The use of evaluation formats is characterized
> by the implementation-defined value of FLT_EVAL_METHOD:
> 19)
>
>   -1 indeterminable;
>   0 evaluate all operations and constants just to the range and precision of the
>     type;
>   1 evaluate operations and constants of type float and double to the
>     range and precision of the double type, evaluate long double
>     operations and constants to the range and precision of the long double
>     type;
>   2 evaluate all operations and constants to the range and precision of the
>     long double type.
>
> All other negative values for FLT_EVAL_METHOD characterize implementation-defined
> behavior.
>
> [2] Definition of float_t and double_t in C99 spec:
>
> C99 Spec draft: (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)
>
> 7.12
>
> ...
>
> The types
> float_t
> double_t
> are floating types at least as wide as float and double, respectively, and such that
> double_t is at least as wide as float_t. If FLT_EVAL_METHOD equals 0,
> float_t and double_t are float and double, respectively; if
> FLT_EVAL_METHOD equals 1, they are both double; if FLT_EVAL_METHOD equals
> 2, they are both long double; and for other values of FLT_EVAL_METHOD, they are
> otherwise implementation-defined.199)
>
> 199) The types float_t and double_t are intended to be the implementation’s most efficient types at
> least as wide as float and double, respectively. For FLT_EVAL_METHOD equal 0, 1, or 2, the
> type float_t is the narrowest type used by the implementation to
> evaluate floating expressions.
> ---
>  bits/flt-eval-method.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/bits/flt-eval-method.h b/bits/flt-eval-method.h
> index 75f57b9a0e..b6be0a1e43 100644
> --- a/bits/flt-eval-method.h
> +++ b/bits/flt-eval-method.h
> @@ -26,14 +26,14 @@
>     -1.  */
>
>  /* In the default version of this header, follow __FLT_EVAL_METHOD__.
> -   -1 is mapped to 2 (considering evaluation as long double to be a
> +   -1 is mapped to 0 (considering evaluation as same precision to be a
>     conservatively safe assumption), and if __FLT_EVAL_METHOD__ is not
>     defined then assume there is no excess precision and use the value
>     0.  */
>
>  #ifdef __FLT_EVAL_METHOD__
>  # if __FLT_EVAL_METHOD__ == -1
> -#  define __GLIBC_FLT_EVAL_METHOD	2
> +#  define __GLIBC_FLT_EVAL_METHOD	0
>  # else
>  #  define __GLIBC_FLT_EVAL_METHOD	__FLT_EVAL_METHOD__
>  # endif

Acked-by: Palmer Dabbelt <palmer@rivosinc.com> # RISC-V

though we should have a global reviewed look at this now that it's 
touching everything.
Kito Cheng March 9, 2023, 2:14 p.m. UTC | #2 
ping


On Sun, Feb 19, 2023 at 2:29 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Thu, 16 Feb 2023 18:26:46 PST (-0800), kito.cheng@sifive.com wrote:
> > ---
> > We were intending to update RISC-V's setting only, but after discussion
> > with Wilco Dijkstra, we decide to change the generic one instead of RISC-V only
> > since it also fix inefficient issue for float_t and double_t.
> > ---
> >
> > __GLIBC_FLT_EVAL_METHOD will effect the definition of float_t and
> > double_t, currently we'll set __GLIBC_FLT_EVAL_METHOD to 2 when
> > __FLT_EVAL_METHOD__ is -1, that means we'll define float_t and double_t
> > to long double.
> >
> > However some target isn't natively (HW) support long double like AArch64 and
> > RISC-V, they defined long double as 128-bits IEEE 754 floating point type.
> >
> > That means setting __GLIBC_FLT_EVAL_METHOD to 2 will cause very inefficient
> > code gen for those target who didn't provide native support for long
> > double, and that's violate the spirit float_t and double_t - most efficient
> > types at least as wide as float and double.
> >
> > So this patch propose to remap __GLIBC_FLT_EVAL_METHOD to 0 rather than
> > 2 when __FLT_EVAL_METHOD__ is -1, which means we'll use float/double
> > rather than long double for float_t and double_t.
> >
> > Note: __FLT_EVAL_METHOD__ == -1 means the precision is indeterminable,
> >       which means compiler might using indeterminable precision during
> >       optimization/code gen, clang will set this value to -1 when fast
> >       math is enabled.
> >
> > Note: Default definition float_t and double_t in current glibc:
> >       |  __GLIBC_FLT_EVAL_METHOD | float_t     | double_t
> >       |               0 or 16    | float       | double
> >       |               1          | double      | doulbe
> >       |               2          | long double | long double
> >       More complete list see math/math.h
> >
> > Note: RISC-V has defined ISA extension to support 128-bits IEEE 754
> >       floating point operations, but only rare RISC-V core will implement that.
> >
> > Related link:
> >
> > [1] LLVM issue (__FLT_EVAL_METHOD__ is set to -1 with Ofast. #60781):
> >     https://github.com/llvm/llvm-project/issues/60781
> > [2] Last version of this patch: https://sourceware.org/pipermail/libc-alpha/2023-February/145622.html
> >
> > Ref:
> >
> > [1] Definition of FLT_EVAL_METHOD from C99 spec:
> > C99 Spec draft: (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)
> >
> > Except for assignment and cast (which remove all extra range and precision), the values
> > of operations with floating operands and values subject to the usual arithmetic
> > conversions and of floating constants are evaluated to a format whose range and precision
> > may be greater than required by the type. The use of evaluation formats is characterized
> > by the implementation-defined value of FLT_EVAL_METHOD:
> > 19)
> >
> >   -1 indeterminable;
> >   0 evaluate all operations and constants just to the range and precision of the
> >     type;
> >   1 evaluate operations and constants of type float and double to the
> >     range and precision of the double type, evaluate long double
> >     operations and constants to the range and precision of the long double
> >     type;
> >   2 evaluate all operations and constants to the range and precision of the
> >     long double type.
> >
> > All other negative values for FLT_EVAL_METHOD characterize implementation-defined
> > behavior.
> >
> > [2] Definition of float_t and double_t in C99 spec:
> >
> > C99 Spec draft: (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)
> >
> > 7.12
> >
> > ...
> >
> > The types
> > float_t
> > double_t
> > are floating types at least as wide as float and double, respectively, and such that
> > double_t is at least as wide as float_t. If FLT_EVAL_METHOD equals 0,
> > float_t and double_t are float and double, respectively; if
> > FLT_EVAL_METHOD equals 1, they are both double; if FLT_EVAL_METHOD equals
> > 2, they are both long double; and for other values of FLT_EVAL_METHOD, they are
> > otherwise implementation-defined.199)
> >
> > 199) The types float_t and double_t are intended to be the implementation’s most efficient types at
> > least as wide as float and double, respectively. For FLT_EVAL_METHOD equal 0, 1, or 2, the
> > type float_t is the narrowest type used by the implementation to
> > evaluate floating expressions.
> > ---
> >  bits/flt-eval-method.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/bits/flt-eval-method.h b/bits/flt-eval-method.h
> > index 75f57b9a0e..b6be0a1e43 100644
> > --- a/bits/flt-eval-method.h
> > +++ b/bits/flt-eval-method.h
> > @@ -26,14 +26,14 @@
> >     -1.  */
> >
> >  /* In the default version of this header, follow __FLT_EVAL_METHOD__.
> > -   -1 is mapped to 2 (considering evaluation as long double to be a
> > +   -1 is mapped to 0 (considering evaluation as same precision to be a
> >     conservatively safe assumption), and if __FLT_EVAL_METHOD__ is not
> >     defined then assume there is no excess precision and use the value
> >     0.  */
> >
> >  #ifdef __FLT_EVAL_METHOD__
> >  # if __FLT_EVAL_METHOD__ == -1
> > -#  define __GLIBC_FLT_EVAL_METHOD    2
> > +#  define __GLIBC_FLT_EVAL_METHOD    0
> >  # else
> >  #  define __GLIBC_FLT_EVAL_METHOD    __FLT_EVAL_METHOD__
> >  # endif
>
> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> # RISC-V
>
> though we should have a global reviewed look at this now that it's
> touching everything.
Xi Ruoyao March 9, 2023, 2:33 p.m. UTC | #3 
I'm wondering why the compiler will set __FLT_EVAL_METHOD__ to -1 in the
first place.  On gcc91 (VisionFive v1 board) I get:

xry111@gcc91:~$ echo __FLT_EVAL_METHOD__ | cpp
# 0 "<stdin>"
# 0 "<built-in>"
# 0 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 0 "<command-line>" 2
# 1 "<stdin>"
0

On Thu, 2023-03-09 at 22:14 +0800, Kito Cheng via Libc-alpha wrote:
> ping
> 
> 
> On Sun, Feb 19, 2023 at 2:29 AM Palmer Dabbelt <palmer@rivosinc.com>
> wrote:
> > 
> > On Thu, 16 Feb 2023 18:26:46 PST (-0800),
> > kito.cheng@sifive.com wrote:
> > > ---
> > > We were intending to update RISC-V's setting only, but after
> > > discussion
> > > with Wilco Dijkstra, we decide to change the generic one instead
> > > of RISC-V only
> > > since it also fix inefficient issue for float_t and double_t.
> > > ---
> > > 
> > > __GLIBC_FLT_EVAL_METHOD will effect the definition of float_t and
> > > double_t, currently we'll set __GLIBC_FLT_EVAL_METHOD to 2 when
> > > __FLT_EVAL_METHOD__ is -1, that means we'll define float_t and
> > > double_t
> > > to long double.
> > > 
> > > However some target isn't natively (HW) support long double like
> > > AArch64 and
> > > RISC-V, they defined long double as 128-bits IEEE 754 floating
> > > point type.
> > > 
> > > That means setting __GLIBC_FLT_EVAL_METHOD to 2 will cause very
> > > inefficient
> > > code gen for those target who didn't provide native support for
> > > long
> > > double, and that's violate the spirit float_t and double_t - most
> > > efficient
> > > types at least as wide as float and double.
> > > 
> > > So this patch propose to remap __GLIBC_FLT_EVAL_METHOD to 0 rather
> > > than
> > > 2 when __FLT_EVAL_METHOD__ is -1, which means we'll use
> > > float/double
> > > rather than long double for float_t and double_t.
> > > 
> > > Note: __FLT_EVAL_METHOD__ == -1 means the precision is
> > > indeterminable,
> > >       which means compiler might using indeterminable precision
> > > during
> > >       optimization/code gen, clang will set this value to -1 when
> > > fast
> > >       math is enabled.
> > > 
> > > Note: Default definition float_t and double_t in current glibc:
> > >       |  __GLIBC_FLT_EVAL_METHOD | float_t     | double_t
> > >       |               0 or 16    | float       | double
> > >       |               1          | double      | doulbe
> > >       |               2          | long double | long double
> > >       More complete list see math/math.h
> > > 
> > > Note: RISC-V has defined ISA extension to support 128-bits IEEE
> > > 754
> > >       floating point operations, but only rare RISC-V core will
> > > implement that.
> > > 
> > > Related link:
> > > 
> > > [1] LLVM issue (__FLT_EVAL_METHOD__ is set to -1 with Ofast.
> > > #60781):
> > >     https://github.com/llvm/llvm-project/issues/60781
> > > [2] Last version of this patch:
> > > https://sourceware.org/pipermail/libc-alpha/2023-February/145622.html
> > > 
> > > Ref:
> > > 
> > > [1] Definition of FLT_EVAL_METHOD from C99 spec:
> > > C99 Spec draft:
> > > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)
> > > 
> > > Except for assignment and cast (which remove all extra range and
> > > precision), the values
> > > of operations with floating operands and values subject to the
> > > usual arithmetic
> > > conversions and of floating constants are evaluated to a format
> > > whose range and precision
> > > may be greater than required by the type. The use of evaluation
> > > formats is characterized
> > > by the implementation-defined value of FLT_EVAL_METHOD:
> > > 19)
> > > 
> > >   -1 indeterminable;
> > >   0 evaluate all operations and constants just to the range and
> > > precision of the
> > >     type;
> > >   1 evaluate operations and constants of type float and double to
> > > the
> > >     range and precision of the double type, evaluate long double
> > >     operations and constants to the range and precision of the
> > > long double
> > >     type;
> > >   2 evaluate all operations and constants to the range and
> > > precision of the
> > >     long double type.
> > > 
> > > All other negative values for FLT_EVAL_METHOD characterize
> > > implementation-defined
> > > behavior.
> > > 
> > > [2] Definition of float_t and double_t in C99 spec:
> > > 
> > > C99 Spec draft:
> > > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)
> > > 
> > > 7.12
> > > 
> > > ...
> > > 
> > > The types
> > > float_t
> > > double_t
> > > are floating types at least as wide as float and double,
> > > respectively, and such that
> > > double_t is at least as wide as float_t. If FLT_EVAL_METHOD equals
> > > 0,
> > > float_t and double_t are float and double, respectively; if
> > > FLT_EVAL_METHOD equals 1, they are both double; if FLT_EVAL_METHOD
> > > equals
> > > 2, they are both long double; and for other values of
> > > FLT_EVAL_METHOD, they are
> > > otherwise implementation-defined.199)
> > > 
> > > 199) The types float_t and double_t are intended to be the
> > > implementation’s most efficient types at
> > > least as wide as float and double, respectively. For
> > > FLT_EVAL_METHOD equal 0, 1, or 2, the
> > > type float_t is the narrowest type used by the implementation to
> > > evaluate floating expressions.
> > > ---
> > >  bits/flt-eval-method.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/bits/flt-eval-method.h b/bits/flt-eval-method.h
> > > index 75f57b9a0e..b6be0a1e43 100644
> > > --- a/bits/flt-eval-method.h
> > > +++ b/bits/flt-eval-method.h
> > > @@ -26,14 +26,14 @@
> > >     -1.  */
> > > 
> > >  /* In the default version of this header, follow
> > > __FLT_EVAL_METHOD__.
> > > -   -1 is mapped to 2 (considering evaluation as long double to be
> > > a
> > > +   -1 is mapped to 0 (considering evaluation as same precision to
> > > be a
> > >     conservatively safe assumption), and if __FLT_EVAL_METHOD__ is
> > > not
> > >     defined then assume there is no excess precision and use the
> > > value
> > >     0.  */
> > > 
> > >  #ifdef __FLT_EVAL_METHOD__
> > >  # if __FLT_EVAL_METHOD__ == -1
> > > -#  define __GLIBC_FLT_EVAL_METHOD    2
> > > +#  define __GLIBC_FLT_EVAL_METHOD    0
> > >  # else
> > >  #  define __GLIBC_FLT_EVAL_METHOD    __FLT_EVAL_METHOD__
> > >  # endif
> > 
> > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> # RISC-V
> > 
> > though we should have a global reviewed look at this now that it's
> > touching everything.
Kito Cheng March 9, 2023, 2:59 p.m. UTC | #4 
Hi Ruoyao:

GCC isn't set that to -1, but clang/LLVM did, see
https://github.com/llvm/llvm-project/issues/60781 and
https://reviews.llvm.org/D121122

Seems like LLVM folks are consider to rever that, but even clang/LLVM
revert that,

the issue still there: should we treat indeterminable precision as
evaluating value as long double?

It's almost no benefit for those targets who have 128 bit long double type.

On Thu, Mar 9, 2023 at 10:33 PM Xi Ruoyao <xry111@xry111.site> wrote:
>
> I'm wondering why the compiler will set __FLT_EVAL_METHOD__ to -1 in the
> first place.  On gcc91 (VisionFive v1 board) I get:
>
> xry111@gcc91:~$ echo __FLT_EVAL_METHOD__ | cpp
> # 0 "<stdin>"
> # 0 "<built-in>"
> # 0 "<command-line>"
> # 1 "/usr/include/stdc-predef.h" 1 3 4
> # 0 "<command-line>" 2
> # 1 "<stdin>"
> 0
>
> On Thu, 2023-03-09 at 22:14 +0800, Kito Cheng via Libc-alpha wrote:
> > ping
> >
> >
> > On Sun, Feb 19, 2023 at 2:29 AM Palmer Dabbelt <palmer@rivosinc.com>
> > wrote:
> > >
> > > On Thu, 16 Feb 2023 18:26:46 PST (-0800),
> > > kito.cheng@sifive.com wrote:
> > > > ---
> > > > We were intending to update RISC-V's setting only, but after
> > > > discussion
> > > > with Wilco Dijkstra, we decide to change the generic one instead
> > > > of RISC-V only
> > > > since it also fix inefficient issue for float_t and double_t.
> > > > ---
> > > >
> > > > __GLIBC_FLT_EVAL_METHOD will effect the definition of float_t and
> > > > double_t, currently we'll set __GLIBC_FLT_EVAL_METHOD to 2 when
> > > > __FLT_EVAL_METHOD__ is -1, that means we'll define float_t and
> > > > double_t
> > > > to long double.
> > > >
> > > > However some target isn't natively (HW) support long double like
> > > > AArch64 and
> > > > RISC-V, they defined long double as 128-bits IEEE 754 floating
> > > > point type.
> > > >
> > > > That means setting __GLIBC_FLT_EVAL_METHOD to 2 will cause very
> > > > inefficient
> > > > code gen for those target who didn't provide native support for
> > > > long
> > > > double, and that's violate the spirit float_t and double_t - most
> > > > efficient
> > > > types at least as wide as float and double.
> > > >
> > > > So this patch propose to remap __GLIBC_FLT_EVAL_METHOD to 0 rather
> > > > than
> > > > 2 when __FLT_EVAL_METHOD__ is -1, which means we'll use
> > > > float/double
> > > > rather than long double for float_t and double_t.
> > > >
> > > > Note: __FLT_EVAL_METHOD__ == -1 means the precision is
> > > > indeterminable,
> > > >       which means compiler might using indeterminable precision
> > > > during
> > > >       optimization/code gen, clang will set this value to -1 when
> > > > fast
> > > >       math is enabled.
> > > >
> > > > Note: Default definition float_t and double_t in current glibc:
> > > >       |  __GLIBC_FLT_EVAL_METHOD | float_t     | double_t
> > > >       |               0 or 16    | float       | double
> > > >       |               1          | double      | doulbe
> > > >       |               2          | long double | long double
> > > >       More complete list see math/math.h
> > > >
> > > > Note: RISC-V has defined ISA extension to support 128-bits IEEE
> > > > 754
> > > >       floating point operations, but only rare RISC-V core will
> > > > implement that.
> > > >
> > > > Related link:
> > > >
> > > > [1] LLVM issue (__FLT_EVAL_METHOD__ is set to -1 with Ofast.
> > > > #60781):
> > > >     https://github.com/llvm/llvm-project/issues/60781
> > > > [2] Last version of this patch:
> > > > https://sourceware.org/pipermail/libc-alpha/2023-February/145622.html
> > > >
> > > > Ref:
> > > >
> > > > [1] Definition of FLT_EVAL_METHOD from C99 spec:
> > > > C99 Spec draft:
> > > > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)
> > > >
> > > > Except for assignment and cast (which remove all extra range and
> > > > precision), the values
> > > > of operations with floating operands and values subject to the
> > > > usual arithmetic
> > > > conversions and of floating constants are evaluated to a format
> > > > whose range and precision
> > > > may be greater than required by the type. The use of evaluation
> > > > formats is characterized
> > > > by the implementation-defined value of FLT_EVAL_METHOD:
> > > > 19)
> > > >
> > > >   -1 indeterminable;
> > > >   0 evaluate all operations and constants just to the range and
> > > > precision of the
> > > >     type;
> > > >   1 evaluate operations and constants of type float and double to
> > > > the
> > > >     range and precision of the double type, evaluate long double
> > > >     operations and constants to the range and precision of the
> > > > long double
> > > >     type;
> > > >   2 evaluate all operations and constants to the range and
> > > > precision of the
> > > >     long double type.
> > > >
> > > > All other negative values for FLT_EVAL_METHOD characterize
> > > > implementation-defined
> > > > behavior.
> > > >
> > > > [2] Definition of float_t and double_t in C99 spec:
> > > >
> > > > C99 Spec draft:
> > > > (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf)
> > > >
> > > > 7.12
> > > >
> > > > ...
> > > >
> > > > The types
> > > > float_t
> > > > double_t
> > > > are floating types at least as wide as float and double,
> > > > respectively, and such that
> > > > double_t is at least as wide as float_t. If FLT_EVAL_METHOD equals
> > > > 0,
> > > > float_t and double_t are float and double, respectively; if
> > > > FLT_EVAL_METHOD equals 1, they are both double; if FLT_EVAL_METHOD
> > > > equals
> > > > 2, they are both long double; and for other values of
> > > > FLT_EVAL_METHOD, they are
> > > > otherwise implementation-defined.199)
> > > >
> > > > 199) The types float_t and double_t are intended to be the
> > > > implementation’s most efficient types at
> > > > least as wide as float and double, respectively. For
> > > > FLT_EVAL_METHOD equal 0, 1, or 2, the
> > > > type float_t is the narrowest type used by the implementation to
> > > > evaluate floating expressions.
> > > > ---
> > > >  bits/flt-eval-method.h | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/bits/flt-eval-method.h b/bits/flt-eval-method.h
> > > > index 75f57b9a0e..b6be0a1e43 100644
> > > > --- a/bits/flt-eval-method.h
> > > > +++ b/bits/flt-eval-method.h
> > > > @@ -26,14 +26,14 @@
> > > >     -1.  */
> > > >
> > > >  /* In the default version of this header, follow
> > > > __FLT_EVAL_METHOD__.
> > > > -   -1 is mapped to 2 (considering evaluation as long double to be
> > > > a
> > > > +   -1 is mapped to 0 (considering evaluation as same precision to
> > > > be a
> > > >     conservatively safe assumption), and if __FLT_EVAL_METHOD__ is
> > > > not
> > > >     defined then assume there is no excess precision and use the
> > > > value
> > > >     0.  */
> > > >
> > > >  #ifdef __FLT_EVAL_METHOD__
> > > >  # if __FLT_EVAL_METHOD__ == -1
> > > > -#  define __GLIBC_FLT_EVAL_METHOD    2
> > > > +#  define __GLIBC_FLT_EVAL_METHOD    0
> > > >  # else
> > > >  #  define __GLIBC_FLT_EVAL_METHOD    __FLT_EVAL_METHOD__
> > > >  # endif
> > >
> > > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> # RISC-V
> > >
> > > though we should have a global reviewed look at this now that it's
> > > touching everything.
>
> --
> Xi Ruoyao <xry111@xry111.site>
> School of Aerospace Science and Technology, Xidian University
Xi Ruoyao March 9, 2023, 3:18 p.m. UTC | #5 
On Thu, 2023-03-09 at 22:59 +0800, Kito Cheng wrote:
> Hi Ruoyao:
> 
> GCC isn't set that to -1, but clang/LLVM did, see
> https://github.com/llvm/llvm-project/issues/60781 and
> https://reviews.llvm.org/D121122

Hmm, it turns -ffast-math into "-fslow-math" :(.

> Seems like LLVM folks are consider to rever that, but even clang/LLVM
> revert that,
> 
> the issue still there: should we treat indeterminable precision as
> evaluating value as long double?
> 
> It's almost no benefit for those targets who have 128 bit long double
> type.

I agree that if __FLT_EVAL_METHOD__ is not 0, 1, or 2, and the target
does not have native support for some "special" floating point types, we
*should* make float_t float and double_t double.

But doing so may blow up rolling-release distros: if a library uses
float_t and double_t in the API and the distro maintainers rebuilt the
library with a new Glibc, but (s)he has not rebuilt an application using
the library yet, the application will just crash or produce "strange"
results.  Maybe we'll need to issue an alert about this to the distro
maintainers.
Kito Cheng March 9, 2023, 4:03 p.m. UTC | #6 
Hi Ruoyao:

> I agree that if __FLT_EVAL_METHOD__ is not 0, 1, or 2, and the target
> does not have native support for some "special" floating point types, we
> *should* make float_t float and double_t double.
>
> But doing so may blow up rolling-release distros: if a library uses
> float_t and double_t in the API and the distro maintainers rebuilt the
> library with a new Glibc, but (s)he has not rebuilt an application using
> the library yet, the application will just crash or produce "strange"
> results.  Maybe we'll need to issue an alert about this to the distro
> maintainers.

This change should not break the API in most cases since my
understanding the fast math mode is not default on, and also
GCC won't emit __FLT_EVAL_METHOD__ == -1 even -ffast-math
is given - only ICC and clang/LLVM did that.

So that should be a safe change for disto I think?
Wilco Dijkstra March 9, 2023, 4:20 p.m. UTC | #7 
Hi,

>> GCC isn't set that to -1, but clang/LLVM did, see
>> https://github.com/llvm/llvm-project/issues/60781 and
>> https://reviews.llvm.org/D121122
>
> Hmm, it turns -ffast-math into "-fslow-math" :(.

Yes that alone is a good reason to fix this!

> I agree that if __FLT_EVAL_METHOD__ is not 0, 1, or 2, and the target
> does not have native support for some "special" floating point types, we
> *should* make float_t float and double_t double.
>
> But doing so may blow up rolling-release distros: if a library uses
> float_t and double_t in the API and the distro maintainers rebuilt the
> library with a new Glibc, but (s)he has not rebuilt an application using
> the library yet, the application will just crash or produce "strange"
> results.  Maybe we'll need to issue an alert about this to the distro
> maintainers.

GCC never uses -1, so distros should be fine. We could backport the GLIBC fix.

It's not clear whether float_t/double_t are allowed on interfaces. On x86 GCC uses
2 by default but LLVM uses 0, and with Ofast x86 GCC uses 0 while LLVM uses -1
(which is mapped to 2 by GLIBC). With LLVM fixed it will be 0.

So it looks like a mess, but on x86 there is precedent for incompatible values
between compilers and optimization settings.

Cheers,
Wilco
Wilco Dijkstra March 9, 2023, 7:37 p.m. UTC | #8 
Hi Kito,

This comment reads a bit strange after the change:

> >  /* In the default version of this header, follow __FLT_EVAL_METHOD__.
> > -   -1 is mapped to 2 (considering evaluation as long double to be a
> > +   -1 is mapped to 0 (considering evaluation as same precision to be a
> >     conservatively safe assumption), and if __FLT_EVAL_METHOD__ is not
> >     defined then assume there is no excess precision and use the value
> >     0.  */

Maybe this would be clearer?

In the default version of this header, follow __FLT_EVAL_METHOD__.
If __FLT_EVAL_METHOD__ is not defined or set to -1, assume there is no
excess precision and use the value 0 (this is correct for most targets).


Also separating the commit message from the rest of the discussion would be
good - I assume the quote from the C99 standard is not part of it?

Cheers,
Wilco
diff mbox series

Patch

diff --git a/bits/flt-eval-method.h b/bits/flt-eval-method.h
index 75f57b9a0e..b6be0a1e43 100644
--- a/bits/flt-eval-method.h
+++ b/bits/flt-eval-method.h
@@ -26,14 +26,14 @@ 
    -1.  */
 
 /* In the default version of this header, follow __FLT_EVAL_METHOD__.
-   -1 is mapped to 2 (considering evaluation as long double to be a
+   -1 is mapped to 0 (considering evaluation as same precision to be a
    conservatively safe assumption), and if __FLT_EVAL_METHOD__ is not
    defined then assume there is no excess precision and use the value
    0.  */
 
 #ifdef __FLT_EVAL_METHOD__
 # if __FLT_EVAL_METHOD__ == -1
-#  define __GLIBC_FLT_EVAL_METHOD	2
+#  define __GLIBC_FLT_EVAL_METHOD	0
 # else
 #  define __GLIBC_FLT_EVAL_METHOD	__FLT_EVAL_METHOD__
 # endif