S390: Derive float_t from FLT_EVAL_METHOD
Commit Message
Hi,
float_t should represent the type that is used to evaluate float
expressions internally. On s390(x), float_t is currently set to
double. In contrast, the isa supports single-precision float
operations and compilers by default evaluate float in single
precision, which violates the C standard (sections 5.2.4.2.2 and 7.12
in C11/C17). With -fexcess-precision=standard, gcc evaluates float in
double precision, which aligns with the standard yet at the cost of
added conversion instructions. This patch changes the definition of
float_t to be derived from the compiler's __FLT_EVAL_METHOD__ and
thereby align with the C standard -- that is, drop the s390-specific
definition and apply default behavior.
For background: The port of glibc to s390 incorrectly deferred to the
generic definitions which, back then, tied float_t to double. Since
then, this definition has been kept to avoid ABI changes, most
recently in the refactoring of float_t into bits/flt-eval-method.h
https://sourceware.org/legacy-ml/libc-alpha/2016-11/msg00903.html
and the discussion around
https://gcc.gnu.org/legacy-ml/gcc-patches/2016-09/msg02392.html
Given the performance overhead, I have reevaluated the impact of
turning float_t into float and only found two packages, ImageMagick
and clucene, that use float_t in their API, out of >130k Debian source
packages scanned. To avoid breaking ABI changes, I patched these
packages to avoid their reliance on float_t (in ImageMagick since
7.0.10-39, patch in
https://github.com/ImageMagick/ImageMagick/pull/2832 - patch for
clucene in https://sourceforge.net/p/clucene/bugs/233). With these
measures in place, we should get rid of the inconsistent definition of
float_t for s390.
Marius
------------>8------------>8------------>8------------>8------------
float_t supposedly represents the type that is used to evaluate float
expressions internally. While the isa supports single-precision float
operations, the port of glibc to s390 incorrectly deferred to the
generic definitions which, back then, tied float_t to double. gcc by
default evaluates float in single precision, so that scenario violates
the C standard (sections 5.2.4.2.2 and 7.12 in C11/C17). With
-fexcess-precision=standard, gcc evaluates float in double precision,
which aligns with the standard yet at the cost of added conversion
instructions.
With this patch, we drop the s390-specific definition of float_t and
defer to the default behavior, which aligns float_t with the
compiler-defined FLT_EVAL_METHOD in a standard-compliant way.
Checked on s390x-linux-gnu with 31-bit and 64-bit builds.
---
NEWS | 7 +++++++
sysdeps/s390/bits/flt-eval-method.h | 24 ------------------------
2 files changed, 7 insertions(+), 24 deletions(-)
delete mode 100644 sysdeps/s390/bits/flt-eval-method.h
Comments
On 11/30/20 3:53 PM, Marius Hillenbrand wrote:
> Hi,
>
> float_t should represent the type that is used to evaluate float
> expressions internally. On s390(x), float_t is currently set to
> double. In contrast, the isa supports single-precision float
> operations and compilers by default evaluate float in single
> precision, which violates the C standard (sections 5.2.4.2.2 and 7.12
> in C11/C17). With -fexcess-precision=standard, gcc evaluates float in
> double precision, which aligns with the standard yet at the cost of
> added conversion instructions. This patch changes the definition of
> float_t to be derived from the compiler's __FLT_EVAL_METHOD__ and
> thereby align with the C standard -- that is, drop the s390-specific
> definition and apply default behavior.
>
>
> For background: The port of glibc to s390 incorrectly deferred to the
> generic definitions which, back then, tied float_t to double. Since
> then, this definition has been kept to avoid ABI changes, most
> recently in the refactoring of float_t into bits/flt-eval-method.h
> https://sourceware.org/legacy-ml/libc-alpha/2016-11/msg00903.html
> and the discussion around
> https://gcc.gnu.org/legacy-ml/gcc-patches/2016-09/msg02392.html
>
> Given the performance overhead, I have reevaluated the impact of
> turning float_t into float and only found two packages, ImageMagick
> and clucene, that use float_t in their API, out of >130k Debian source
> packages scanned. To avoid breaking ABI changes, I patched these
> packages to avoid their reliance on float_t (in ImageMagick since
> 7.0.10-39, patch in
> https://github.com/ImageMagick/ImageMagick/pull/2832 - patch for
> clucene in https://sourceforge.net/p/clucene/bugs/233). With these
> measures in place, we should get rid of the inconsistent definition of
> float_t for s390.
>
> Marius
>
>
> ------------>8------------>8------------>8------------>8------------
> float_t supposedly represents the type that is used to evaluate float
> expressions internally. While the isa supports single-precision float
> operations, the port of glibc to s390 incorrectly deferred to the
> generic definitions which, back then, tied float_t to double. gcc by
> default evaluates float in single precision, so that scenario violates
> the C standard (sections 5.2.4.2.2 and 7.12 in C11/C17). With
> -fexcess-precision=standard, gcc evaluates float in double precision,
> which aligns with the standard yet at the cost of added conversion
> instructions.
>
> With this patch, we drop the s390-specific definition of float_t and
> defer to the default behavior, which aligns float_t with the
> compiler-defined FLT_EVAL_METHOD in a standard-compliant way.
>
> Checked on s390x-linux-gnu with 31-bit and 64-bit builds.
> ---
> NEWS | 7 +++++++
> sysdeps/s390/bits/flt-eval-method.h | 24 ------------------------
> 2 files changed, 7 insertions(+), 24 deletions(-)
> delete mode 100644 sysdeps/s390/bits/flt-eval-method.h
>
> diff --git a/NEWS b/NEWS
> index 685b93c8e9..6bec44d807 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -46,6 +46,13 @@ Deprecated and removed features, and other changes affecting compatibility:
> program is now installed in the /usr/bin subdirectory. Previously,
> the /usr/sbin subdirectory was used.
>
> +* On s390(x), the type float_t is now derived from the macro
> + __FLT_EVAL_METHOD__ that is defined by the compiler, instead of being
> + hardcoded to double. This does not affect the ABI of any libraries
> + that are part of the GNU C Library, but may affect the ABI of other
> + libraries that use this type in their interfaces. The new definition
> + improves consistency with compiler behavior in many scenarios.
> +
> Changes to build and runtime requirements:
>
> * On Linux, the system administrator needs to configure /dev/pts with
> diff --git a/sysdeps/s390/bits/flt-eval-method.h b/sysdeps/s390/bits/flt-eval-method.h
> deleted file mode 100644
> index 826bbfec2f..0000000000
> --- a/sysdeps/s390/bits/flt-eval-method.h
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -/* Define __GLIBC_FLT_EVAL_METHOD. S/390 version.
> - Copyright (C) 2016-2020 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <https://www.gnu.org/licenses/>. */
> -
> -#ifndef _MATH_H
> -# error "Never use <bits/flt-eval-method.h> directly; include <math.h> instead."
> -#endif
> -
> -/* This value is used because of a historical mistake. */
> -#define __GLIBC_FLT_EVAL_METHOD 1
>
Hi,
this patch is okay. I've committed it:
"S390: Derive float_t from FLT_EVAL_METHOD"
https://sourceware.org/git/?p=glibc.git;a=commit;h=f88242af19dc970949806790f70c6fd6336944a6
Thanks,
Stefan
@@ -46,6 +46,13 @@ Deprecated and removed features, and other changes affecting compatibility:
program is now installed in the /usr/bin subdirectory. Previously,
the /usr/sbin subdirectory was used.
+* On s390(x), the type float_t is now derived from the macro
+ __FLT_EVAL_METHOD__ that is defined by the compiler, instead of being
+ hardcoded to double. This does not affect the ABI of any libraries
+ that are part of the GNU C Library, but may affect the ABI of other
+ libraries that use this type in their interfaces. The new definition
+ improves consistency with compiler behavior in many scenarios.
+
Changes to build and runtime requirements:
* On Linux, the system administrator needs to configure /dev/pts with
deleted file mode 100644
@@ -1,24 +0,0 @@
-/* Define __GLIBC_FLT_EVAL_METHOD. S/390 version.
- Copyright (C) 2016-2020 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
-
- The GNU C Library is free software; you can redistribute it and/or
- modify it under the terms of the GNU Lesser General Public
- License as published by the Free Software Foundation; either
- version 2.1 of the License, or (at your option) any later version.
-
- The GNU C Library is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- Lesser General Public License for more details.
-
- You should have received a copy of the GNU Lesser General Public
- License along with the GNU C Library; if not, see
- <https://www.gnu.org/licenses/>. */
-
-#ifndef _MATH_H
-# error "Never use <bits/flt-eval-method.h> directly; include <math.h> instead."
-#endif
-
-/* This value is used because of a historical mistake. */
-#define __GLIBC_FLT_EVAL_METHOD 1