S390: Derive float_t from FLT_EVAL_METHOD

  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
  

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