Message ID | 20201130145359.2831-1-mhillen@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | S390: Derive float_t from FLT_EVAL_METHOD | expand |
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
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