Message ID | 20200309183234.11891-1-adhemerval.zanella@linaro.org |
---|---|
State | Accepted |
Commit | 5f34491510efe37d094c1fca66c7404002cdcdc5 |
Headers | show |
Series | [v2,1/3] math: Remove fenvinline.h | expand |
On 3/9/20 1:32 PM, Adhemerval Zanella wrote: > Changes from previous version: > > - Mention on commit message x86 also exports a similar optimization, > but on a different header. > > -- > > Similar to string2.h (18b10de7ce) and string3.h (09a596cc2c) this > patch removes the fenvinline.h on all architectures. Currently > only powerpc implements some optimizations. This kind of optimization > is better implemented by the compiler (which handles the architecture > ISA transparently). > > Also, for the specific optimized powerpc implementation the code is > becoming convoluted and these micro-optimization are hardly wildly > used, even more being a possible hotspot in realword cases > (non-default rounding are used only on specific cases and exception > handling are done most likely only on errors path). Only x86 > implements similar optimization (on fenv.h) also indicates that > these should no be on libc. > > The math/test-fenv already covers all math/test-fenvinline tests, > so it is safe to remove it. > > Checked on x86_64-linux-gnu and powerpc64le-linux-gnu. > --- > bits/fenvinline.h | 8 - > math/Makefile | 4 +- > math/fenv.h | 4 - > math/test-fenvinline.c | 354 ------------------------------ > sysdeps/powerpc/bits/fenvinline.h | 108 --------- > 5 files changed, 2 insertions(+), 476 deletions(-) > delete mode 100644 bits/fenvinline.h > delete mode 100644 math/test-fenvinline.c > delete mode 100644 sysdeps/powerpc/bits/fenvinline.h Does sysdeps/powerpc/fpu/fegetround.c also need updated with this patch too? I think it using the removed __fegetround macro.
* Adhemerval Zanella: > Similar to string2.h (18b10de7ce) and string3.h (09a596cc2c) this > patch removes the fenvinline.h on all architectures. Currently > only powerpc implements some optimizations. This kind of optimization > is better implemented by the compiler (which handles the architecture > ISA transparently). > > Also, for the specific optimized powerpc implementation the code is > becoming convoluted and these micro-optimization are hardly wildly > used, even more being a possible hotspot in realword cases > (non-default rounding are used only on specific cases and exception > handling are done most likely only on errors path). Only x86 > implements similar optimization (on fenv.h) also indicates that > these should no be on libc. Wasn't the idea to make this change only after the GCC optimizations have been implemented?
On 09/03/2020 17:19, Paul E Murphy wrote: > > > On 3/9/20 1:32 PM, Adhemerval Zanella wrote: >> Changes from previous version: >> >> - Mention on commit message x86 also exports a similar optimization, >> but on a different header. >> >> -- >> >> Similar to string2.h (18b10de7ce) and string3.h (09a596cc2c) this >> patch removes the fenvinline.h on all architectures. Currently >> only powerpc implements some optimizations. This kind of optimization >> is better implemented by the compiler (which handles the architecture >> ISA transparently). >> >> Also, for the specific optimized powerpc implementation the code is >> becoming convoluted and these micro-optimization are hardly wildly >> used, even more being a possible hotspot in realword cases >> (non-default rounding are used only on specific cases and exception >> handling are done most likely only on errors path). Only x86 >> implements similar optimization (on fenv.h) also indicates that >> these should no be on libc. >> >> The math/test-fenv already covers all math/test-fenvinline tests, >> so it is safe to remove it. >> >> Checked on x86_64-linux-gnu and powerpc64le-linux-gnu. >> --- >> bits/fenvinline.h | 8 - >> math/Makefile | 4 +- >> math/fenv.h | 4 - >> math/test-fenvinline.c | 354 ------------------------------ >> sysdeps/powerpc/bits/fenvinline.h | 108 --------- >> 5 files changed, 2 insertions(+), 476 deletions(-) >> delete mode 100644 bits/fenvinline.h >> delete mode 100644 math/test-fenvinline.c >> delete mode 100644 sysdeps/powerpc/bits/fenvinline.h Indeed, I misread the failures on powerpc64le-linux-gnu. Below it is an updated patch with the fegetround optimization moved to an internal header. -- diff --git a/bits/fenvinline.h b/bits/fenvinline.h deleted file mode 100644 index 42f77b5618..0000000000 --- a/bits/fenvinline.h +++ /dev/null @@ -1,8 +0,0 @@ -/* This file provides inline versions of floating-pint environment - handling functions. If there were any. */ - -#ifndef __NO_MATH_INLINES - -/* Here is where the code would go. */ - -#endif diff --git a/math/Makefile b/math/Makefile index 84a8b94c74..f916594a51 100644 --- a/math/Makefile +++ b/math/Makefile @@ -24,7 +24,7 @@ include ../Makeconfig # Installed header files. headers := math.h bits/mathcalls.h bits/mathinline.h \ fpu_control.h complex.h bits/cmathcalls.h fenv.h \ - bits/fenv.h bits/fenvinline.h bits/mathdef.h tgmath.h \ + bits/fenv.h bits/mathdef.h tgmath.h \ bits/math-vector.h finclude/math-vector-fortran.h \ bits/libm-simd-decl-stubs.h bits/iscanonical.h \ bits/flt-eval-method.h bits/fp-fast.h bits/fp-logb.h \ @@ -233,7 +233,7 @@ tests = test-matherr-3 test-fenv basic-test \ test-misc test-fpucw test-fpucw-ieee tst-definitions test-tgmath \ test-tgmath-ret bug-nextafter bug-nexttoward bug-tgmath1 \ test-tgmath-int test-tgmath2 test-powl tst-CMPLX tst-CMPLX2 test-snan \ - test-fenv-tls test-fenv-preserve test-fenv-return test-fenvinline \ + test-fenv-tls test-fenv-preserve test-fenv-return \ test-nearbyint-except test-fenv-clear \ test-nearbyint-except-2 test-signgam-uchar test-signgam-uchar-init \ test-signgam-uint test-signgam-uint-init test-signgam-ullong \ diff --git a/math/fenv.h b/math/fenv.h index 6cad1d3575..e6b9578d6c 100644 --- a/math/fenv.h +++ b/math/fenv.h @@ -140,10 +140,6 @@ extern int fegetmode (femode_t *__modep) __THROW; extern int fesetmode (const femode_t *__modep) __THROW; #endif -/* Include optimization. */ -#ifdef __OPTIMIZE__ -# include <bits/fenvinline.h> -#endif /* NaN support. */ diff --git a/math/test-fenvinline.c b/math/test-fenvinline.c deleted file mode 100644 index 0e5d361fff..0000000000 --- a/math/test-fenvinline.c +++ /dev/null @@ -1,354 +0,0 @@ -/* Test for fenv inline implementations. - Copyright (C) 2015-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 _GNU_SOURCE -# define _GNU_SOURCE -#endif - -/* To make sure the fenv inline function are used. */ -#undef __NO_MATH_INLINES - -#include <fenv.h> -#include <stdio.h> -#include <math-tests.h> - -/* - Since not all architectures might define all exceptions, we define - a private set and map accordingly. -*/ -#define NO_EXC 0 -#define INEXACT_EXC 0x1 -#define DIVBYZERO_EXC 0x2 -#define UNDERFLOW_EXC 0x04 -#define OVERFLOW_EXC 0x08 -#define INVALID_EXC 0x10 -#define ALL_EXC \ - (INEXACT_EXC | DIVBYZERO_EXC | UNDERFLOW_EXC | OVERFLOW_EXC \ - | INVALID_EXC) -static int count_errors; - -#if FE_ALL_EXCEPT -static void -test_single_exception_fp_int (int exception, - int exc_flag, - int fe_flag, - const char *flag_name) -{ - if (exception & exc_flag) - { - if (fetestexcept (fe_flag)) - printf (" Pass: Exception \"%s\" is set\n", flag_name); - else - { - printf (" Fail: Exception \"%s\" is not set\n", flag_name); - ++count_errors; - } - } - else - { - if (fetestexcept (fe_flag)) - { - printf (" Fail: Exception \"%s\" is set\n", flag_name); - ++count_errors; - } - else - printf (" Pass: Exception \"%s\" is not set\n", flag_name); - } -} -/* Test whether a given exception was raised. */ -static void -test_single_exception_fp_double (int exception, - int exc_flag, - double fe_flag, - const char *flag_name) -{ - if (exception & exc_flag) - { - if (fetestexcept (fe_flag)) - printf (" Pass: Exception \"%s\" is set\n", flag_name); - else - { - printf (" Fail: Exception \"%s\" is not set\n", flag_name); - ++count_errors; - } - } - else - { - if (fetestexcept (fe_flag)) - { - printf (" Fail: Exception \"%s\" is set\n", flag_name); - ++count_errors; - } - else - printf (" Pass: Exception \"%s\" is not set\n", flag_name); - } -} -#endif - -static void -test_exceptions (const char *test_name, int exception) -{ - printf ("Test: %s\n", test_name); -#ifdef FE_DIVBYZERO - test_single_exception_fp_double (exception, DIVBYZERO_EXC, FE_DIVBYZERO, - "DIVBYZERO"); -#endif -#ifdef FE_INVALID - test_single_exception_fp_double (exception, INVALID_EXC, FE_INVALID, - "INVALID"); -#endif -#ifdef FE_INEXACT - test_single_exception_fp_double (exception, INEXACT_EXC, FE_INEXACT, - "INEXACT"); -#endif -#ifdef FE_UNDERFLOW - test_single_exception_fp_double (exception, UNDERFLOW_EXC, FE_UNDERFLOW, - "UNDERFLOW"); -#endif -#ifdef FE_OVERFLOW - test_single_exception_fp_double (exception, OVERFLOW_EXC, FE_OVERFLOW, - "OVERFLOW"); -#endif -} - -static void -test_exceptionflag (void) -{ - printf ("Test: fegetexceptionflag (FE_ALL_EXCEPT)\n"); -#if FE_ALL_EXCEPT - fexcept_t excepts; - - feclearexcept (FE_ALL_EXCEPT); - - feraiseexcept (FE_INVALID); - fegetexceptflag (&excepts, FE_ALL_EXCEPT); - - feclearexcept (FE_ALL_EXCEPT); - feraiseexcept (FE_OVERFLOW | FE_INEXACT); - - fesetexceptflag (&excepts, FE_ALL_EXCEPT); - - test_single_exception_fp_int (INVALID_EXC, INVALID_EXC, FE_INVALID, - "INVALID (int)"); - test_single_exception_fp_int (INVALID_EXC, OVERFLOW_EXC, FE_OVERFLOW, - "OVERFLOW (int)"); - test_single_exception_fp_int (INVALID_EXC, INEXACT_EXC, FE_INEXACT, - "INEXACT (int)"); - - /* Same test, but using double as argument */ - feclearexcept (FE_ALL_EXCEPT); - - feraiseexcept (FE_INVALID); - fegetexceptflag (&excepts, (double)FE_ALL_EXCEPT); - - feclearexcept (FE_ALL_EXCEPT); - feraiseexcept (FE_OVERFLOW | FE_INEXACT); - - fesetexceptflag (&excepts, (double)FE_ALL_EXCEPT); - - test_single_exception_fp_double (INVALID_EXC, INVALID_EXC, FE_INVALID, - "INVALID (double)"); - test_single_exception_fp_double (INVALID_EXC, OVERFLOW_EXC, FE_OVERFLOW, - "OVERFLOW (double)"); - test_single_exception_fp_double (INVALID_EXC, INEXACT_EXC, FE_INEXACT, - "INEXACT (double)"); -#endif -} - -static void -test_fesetround (void) -{ -#if defined FE_TONEAREST && defined FE_TOWARDZERO - int res1; - int res2; - - printf ("Tests for fesetround\n"); - - /* The fesetround should not itself cause the test to fail, however it - should either succeed for both 'int' and 'double' argument, or fail - for both. */ - res1 = fesetround ((int) FE_TOWARDZERO); - res2 = fesetround ((double) FE_TOWARDZERO); - if (res1 != res2) - { - printf ("fesetround (FE_TOWARDZERO) failed: %d, %d\n", res1, res2); - ++count_errors; - } - - res1 = fesetround ((int) FE_TONEAREST); - res2 = fesetround ((double) FE_TONEAREST); - if (res1 != res2) - { - printf ("fesetround (FE_TONEAREST) failed: %d, %d\n", res1, res2); - ++count_errors; - } -#endif -} - -#if FE_ALL_EXCEPT -/* Tests for feenableexcept/fedisableexcept. */ -static void -feenable_test (const char *flag_name, fexcept_t fe_exc) -{ - int fe_exci = fe_exc; - double fe_excd = fe_exc; - int excepts; - - /* First disable all exceptions. */ - if (fedisableexcept (FE_ALL_EXCEPT) == -1) - { - printf ("Test: fedisableexcept (FE_ALL_EXCEPT) failed\n"); - ++count_errors; - /* If this fails, the other tests don't make sense. */ - return; - } - - /* Test for inline macros using integer argument. */ - excepts = feenableexcept (fe_exci); - if (!EXCEPTION_ENABLE_SUPPORTED (fe_exci) && excepts == -1) - { - printf ("Test: not testing feenableexcept, it isn't implemented.\n"); - return; - } - if (excepts == -1) - { - printf ("Test: feenableexcept (%s) failed\n", flag_name); - ++count_errors; - return; - } - if (excepts != 0) - { - printf ("Test: feenableexcept (%s) failed, return should be 0, is %x\n", - flag_name, excepts); - ++count_errors; - } - - /* And now disable the exception again. */ - excepts = fedisableexcept (fe_exc); - if (excepts == -1) - { - printf ("Test: fedisableexcept (%s) failed\n", flag_name); - ++count_errors; - return; - } - if (excepts != fe_exc) - { - printf ("Test: fedisableexcept (%s) failed, return should be 0x%x, is 0x%x\n", - flag_name, (unsigned int)fe_exc, excepts); - ++count_errors; - } - - /* Test for inline macros using double argument. */ - excepts = feenableexcept (fe_excd); - if (!EXCEPTION_ENABLE_SUPPORTED (fe_excd) && excepts == -1) - { - printf ("Test: not testing feenableexcept, it isn't implemented.\n"); - return; - } - if (excepts == -1) - { - printf ("Test: feenableexcept (%s) failed\n", flag_name); - ++count_errors; - return; - } - if (excepts != 0) - { - printf ("Test: feenableexcept (%s) failed, return should be 0, is %x\n", - flag_name, excepts); - ++count_errors; - } - - /* And now disable the exception again. */ - excepts = fedisableexcept (fe_exc); - if (excepts == -1) - { - printf ("Test: fedisableexcept (%s) failed\n", flag_name); - ++count_errors; - return; - } - if (excepts != fe_exc) - { - printf ("Test: fedisableexcept (%s) failed, return should be 0x%x, is 0x%x\n", - flag_name, (unsigned int)fe_exc, excepts); - ++count_errors; - } -} -#endif - -static void -test_feenabledisable (void) -{ - printf ("Tests for feenableexcepts/fedisableexcept\n"); - - /* We might have some exceptions still set. */ - feclearexcept (FE_ALL_EXCEPT); - -#ifdef FE_DIVBYZERO - feenable_test ("FE_DIVBYZERO", FE_DIVBYZERO); -#endif -#ifdef FE_INVALID - feenable_test ("FE_INVALID", FE_INVALID); -#endif -#ifdef FE_INEXACT - feenable_test ("FE_INEXACT", FE_INEXACT); -#endif -#ifdef FE_UNDERFLOW - feenable_test ("FE_UNDERFLOW", FE_UNDERFLOW); -#endif -#ifdef FE_OVERFLOW - feenable_test ("FE_OVERFLOW", FE_OVERFLOW); -#endif - fesetenv (FE_DFL_ENV); -} - -static int -do_test (void) -{ - /* clear all exceptions and test if all are cleared */ - feclearexcept (FE_ALL_EXCEPT); - test_exceptions ("feclearexcept (FE_ALL_EXCEPT) clears all exceptions", - NO_EXC); - - /* raise all exceptions and test if all are raised */ - feraiseexcept (FE_ALL_EXCEPT); - if (EXCEPTION_TESTS (float)) - test_exceptions ("feraiseexcept (FE_ALL_EXCEPT) raises all exceptions", - ALL_EXC); - - /* Same test, but using double as argument */ - feclearexcept ((double)FE_ALL_EXCEPT); - test_exceptions ("feclearexcept ((double)FE_ALL_EXCEPT) clears all exceptions", - NO_EXC); - - feraiseexcept ((double)FE_ALL_EXCEPT); - if (EXCEPTION_TESTS (float)) - test_exceptions ("feraiseexcept ((double)FE_ALL_EXCEPT) raises all exceptions", - ALL_EXC); - - if (EXCEPTION_TESTS (float)) - test_exceptionflag (); - - test_fesetround (); - - test_feenabledisable (); - - return count_errors; -} - -#define TEST_FUNCTION do_test () -#include "../test-skeleton.c" diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h deleted file mode 100644 index f2d095a72f..0000000000 --- a/sysdeps/powerpc/bits/fenvinline.h +++ /dev/null @@ -1,108 +0,0 @@ -/* Inline floating-point environment handling functions for powerpc. - Copyright (C) 1995-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/>. */ - -#if defined __GNUC__ && !defined _SOFT_FLOAT && !defined __NO_FPRS__ - -/* Inline definitions for fegetround. */ -# define __fegetround_ISA300() \ - (__extension__ ({ \ - union { double __d; unsigned long long __ll; } __u; \ - __asm__ __volatile__ ( \ - ".machine push; .machine \"power9\"; mffsl %0; .machine pop" \ - : "=f" (__u.__d)); \ - __u.__ll & 0x0000000000000003LL; \ - })) - -# define __fegetround_ISA2() \ - (__extension__ ({ \ - int __fegetround_result; \ - __asm__ __volatile__ ("mcrfs 7,7 ; mfcr %0" \ - : "=r"(__fegetround_result) : : "cr7"); \ - __fegetround_result & 3; \ - })) - -# ifdef _ARCH_PWR9 -# define __fegetround() __fegetround_ISA300() -# elif defined __BUILTIN_CPU_SUPPORTS__ -# define __fegetround() \ - (__glibc_likely (__builtin_cpu_supports ("arch_3_00")) \ - ? __fegetround_ISA300() \ - : __fegetround_ISA2() \ - ) -# else -# define __fegetround() __fegetround_ISA2() -# endif - -# define fegetround() __fegetround () - -# ifndef __NO_MATH_INLINES - -/* Builtins to mtfsb0 and mtfsb1 was introduced on GCC 9. */ -# if !__GNUC_PREREQ(9, 0) -/* The weird 'i#*X' constraints on the following suppress a gcc - warning when __excepts is not a constant. Otherwise, they mean the - same as just plain 'i'. This warning only happens in old GCC - versions (gcc 3 or less). Otherwise plain 'i' works fine. */ -# define __MTFSB0(__b) __asm__ __volatile__ ("mtfsb0 %0" : : "i#*X" (__b)) -# define __MTFSB1(__b) __asm__ __volatile__ ("mtfsb1 %0" : : "i#*X" (__b)) -# else -# define __MTFSB0(__b) __builtin_mtfsb0 (__b) -# define __MTFSB1(__b) __builtin_mtfsb1 (__b) -# endif - -# if __GNUC_PREREQ(3, 4) - -#include <sys/param.h> - -/* Inline definition for feraiseexcept. */ -# define feraiseexcept(__excepts) \ - (__extension__ ({ \ - int __e = __excepts; \ - int __ret = 0; \ - if (__builtin_constant_p (__e) \ - && __builtin_popcount (__e) == 1 \ - && __e != FE_INVALID) \ - { \ - __MTFSB1 ((__builtin_clz (__e))); \ - } \ - else \ - __ret = feraiseexcept (__e); \ - __ret; \ - })) - -/* Inline definition for feclearexcept. */ -# define feclearexcept(__excepts) \ - (__extension__ ({ \ - int __e = __excepts; \ - int __ret = 0; \ - if (__builtin_constant_p (__e) \ - && __builtin_popcount (__e) == 1 \ - && __e != FE_INVALID) \ - { \ - __MTFSB0 ((__builtin_clz (__e))); \ - } \ - else \ - __ret = feclearexcept (__e); \ - __ret; \ - })) - -# endif /* __GNUC_PREREQ(3, 4). */ - -# endif /* !__NO_MATH_INLINES. */ - -#endif /* __GNUC__ && !_SOFT_FLOAT && !__NO_FPRS__ */ diff --git a/sysdeps/powerpc/fpu/fegetround.c b/sysdeps/powerpc/fpu/fegetround.c index 00b4462624..9d7762f08b 100644 --- a/sysdeps/powerpc/fpu/fegetround.c +++ b/sysdeps/powerpc/fpu/fegetround.c @@ -21,10 +21,8 @@ int (__fegetround) (void) { - return __fegetround(); + return __fegetround_inline (); } -#undef fegetround -#undef __fegetround libm_hidden_def (__fegetround) weak_alias (__fegetround, fegetround) libm_hidden_weak (fegetround) diff --git a/sysdeps/powerpc/fpu/fenv_libc.h b/sysdeps/powerpc/fpu/fenv_libc.h index e888c6621c..09dbd3e2df 100644 --- a/sysdeps/powerpc/fpu/fenv_libc.h +++ b/sysdeps/powerpc/fpu/fenv_libc.h @@ -68,6 +68,14 @@ extern const fenv_t *__fe_mask_env (void) attribute_hidden; __fr; \ }) +#define __fe_mffsl() \ + ({register fenv_union_t __fr; \ + __asm__ __volatile__ ( \ + ".machine push; .machine \"power9\"; mffsl %0; .machine pop" \ + : "=f" (__fr.fenv)); \ + __fr.l & 0x3; \ + }) + #define __fe_mffscrn(rn) \ ({register fenv_union_t __fr; \ if (__builtin_constant_p (rn)) \ @@ -144,6 +152,20 @@ typedef union unsigned long long l; } fenv_union_t; +static inline int +__fegetround_inline (void) +{ +#ifdef _ARCH_PWR9 + return __fe_mffsl (); +#else + if (__glibc_likely (GLRO(dl_hwcap2) & PPC_FEATURE2_ARCH_3_00)) + return __fe_mffsl (); + + fenv_union_t fe; + fe.fenv = fegetenv_register (); + return fe.l & 0x3; +#endif +} static inline int __fesetround_inline (int round) diff --git a/sysdeps/powerpc/fpu/fraiseexcpt.c b/sysdeps/powerpc/fpu/fraiseexcpt.c index daa13de500..1bcd31ae14 100644 --- a/sysdeps/powerpc/fpu/fraiseexcpt.c +++ b/sysdeps/powerpc/fpu/fraiseexcpt.c @@ -18,7 +18,6 @@ #include <fenv_libc.h> -#undef feraiseexcept int __feraiseexcept (int excepts) { diff --git a/sysdeps/powerpc/nofpu/fraiseexcpt.c b/sysdeps/powerpc/nofpu/fraiseexcpt.c index 2193a604e2..e60ed68048 100644 --- a/sysdeps/powerpc/nofpu/fraiseexcpt.c +++ b/sysdeps/powerpc/nofpu/fraiseexcpt.c @@ -21,7 +21,6 @@ #include "soft-supp.h" #include <signal.h> -#undef feraiseexcept int __feraiseexcept (int x) {
On 10/03/2020 08:29, Florian Weimer wrote: > * Adhemerval Zanella: > >> Similar to string2.h (18b10de7ce) and string3.h (09a596cc2c) this >> patch removes the fenvinline.h on all architectures. Currently >> only powerpc implements some optimizations. This kind of optimization >> is better implemented by the compiler (which handles the architecture >> ISA transparently). >> >> Also, for the specific optimized powerpc implementation the code is >> becoming convoluted and these micro-optimization are hardly wildly >> used, even more being a possible hotspot in realword cases >> (non-default rounding are used only on specific cases and exception >> handling are done most likely only on errors path). Only x86 >> implements similar optimization (on fenv.h) also indicates that >> these should no be on libc. > > Wasn't the idea to make this change only after the GCC optimizations > have been implemented? > That was Tulio suggestion, but I think this fix is orthogonal because these fenv optimizations shows similar issues as for string{2,3}.h: - They are no scalable, specially for architecture that provides different floating-point environment (such as hard and soft floating point such as ARM) or even hardware revision that aims to improve some performance deficiencies in initial revisions (such as POWER with ISA 3.0). - Newer code avoid setting exceptions directly since they are costly and set on error case. For instance, the new generic logf, log2f and powf implementation that do not support SVID compat. - Exceptions handling are hardly a hotspot, usually it is set in code cold path. This is also likely for non-default rouding mode (since set/reset usually requires flush the pipeline on most FPU implementation).
On Tue, 10 Mar 2020, Florian Weimer wrote: > Wasn't the idea to make this change only after the GCC optimizations > have been implemented? What I did for bits/mathinline.h was file GCC bugs for the optimizations being removed (if not already in GCC), but not wait for them to be fixed. (The only reason bits/mathinline.h now exists at all is that the m68k version is tied into the build of the out-of-line functions for m68k, apart from any cases where GCC might not currently know how to inline the functions for m68k. That dependency on bits/mathinline.h also means building glibc with -Os is broken for m68k, or was the last time I tried building all architectures with -Os.) -- Joseph S. Myers joseph@codesourcery.com
On 10/03/2020 19:54, Joseph Myers wrote: > On Tue, 10 Mar 2020, Florian Weimer wrote: > >> Wasn't the idea to make this change only after the GCC optimizations >> have been implemented? > > What I did for bits/mathinline.h was file GCC bugs for the optimizations > being removed (if not already in GCC), but not wait for them to be fixed. > Alright, I have opened both https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94193 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94194 And updated the commit message locally to reference it. > (The only reason bits/mathinline.h now exists at all is that the m68k > version is tied into the build of the out-of-line functions for m68k, > apart from any cases where GCC might not currently know how to inline the > functions for m68k. That dependency on bits/mathinline.h also means > building glibc with -Os is broken for m68k, or was the last time I tried > building all architectures with -Os.) > For the m68k case I think moving the required macros from mathinline.h to mathimpl.h should enable the build (and it should make Os work as well).
On 10/03/2020 13:51, Adhemerval Zanella wrote: > > > On 09/03/2020 17:19, Paul E Murphy wrote: >> >> >> On 3/9/20 1:32 PM, Adhemerval Zanella wrote: >>> Changes from previous version: >>> >>> - Mention on commit message x86 also exports a similar optimization, >>> but on a different header. >>> >>> -- >>> >>> Similar to string2.h (18b10de7ce) and string3.h (09a596cc2c) this >>> patch removes the fenvinline.h on all architectures. Currently >>> only powerpc implements some optimizations. This kind of optimization >>> is better implemented by the compiler (which handles the architecture >>> ISA transparently). >>> >>> Also, for the specific optimized powerpc implementation the code is >>> becoming convoluted and these micro-optimization are hardly wildly >>> used, even more being a possible hotspot in realword cases >>> (non-default rounding are used only on specific cases and exception >>> handling are done most likely only on errors path). Only x86 >>> implements similar optimization (on fenv.h) also indicates that >>> these should no be on libc. >>> >>> The math/test-fenv already covers all math/test-fenvinline tests, >>> so it is safe to remove it. >>> >>> Checked on x86_64-linux-gnu and powerpc64le-linux-gnu. >>> --- >>> bits/fenvinline.h | 8 - >>> math/Makefile | 4 +- >>> math/fenv.h | 4 - >>> math/test-fenvinline.c | 354 ------------------------------ >>> sysdeps/powerpc/bits/fenvinline.h | 108 --------- >>> 5 files changed, 2 insertions(+), 476 deletions(-) >>> delete mode 100644 bits/fenvinline.h >>> delete mode 100644 math/test-fenvinline.c >>> delete mode 100644 sysdeps/powerpc/bits/fenvinline.h > > Indeed, I misread the failures on powerpc64le-linux-gnu. Below it is > an updated patch with the fegetround optimization moved to an internal > header. Ping. > > -- > > diff --git a/bits/fenvinline.h b/bits/fenvinline.h > deleted file mode 100644 > index 42f77b5618..0000000000 > --- a/bits/fenvinline.h > +++ /dev/null > @@ -1,8 +0,0 @@ > -/* This file provides inline versions of floating-pint environment > - handling functions. If there were any. */ > - > -#ifndef __NO_MATH_INLINES > - > -/* Here is where the code would go. */ > - > -#endif > diff --git a/math/Makefile b/math/Makefile > index 84a8b94c74..f916594a51 100644 > --- a/math/Makefile > +++ b/math/Makefile > @@ -24,7 +24,7 @@ include ../Makeconfig > # Installed header files. > headers := math.h bits/mathcalls.h bits/mathinline.h \ > fpu_control.h complex.h bits/cmathcalls.h fenv.h \ > - bits/fenv.h bits/fenvinline.h bits/mathdef.h tgmath.h \ > + bits/fenv.h bits/mathdef.h tgmath.h \ > bits/math-vector.h finclude/math-vector-fortran.h \ > bits/libm-simd-decl-stubs.h bits/iscanonical.h \ > bits/flt-eval-method.h bits/fp-fast.h bits/fp-logb.h \ > @@ -233,7 +233,7 @@ tests = test-matherr-3 test-fenv basic-test \ > test-misc test-fpucw test-fpucw-ieee tst-definitions test-tgmath \ > test-tgmath-ret bug-nextafter bug-nexttoward bug-tgmath1 \ > test-tgmath-int test-tgmath2 test-powl tst-CMPLX tst-CMPLX2 test-snan \ > - test-fenv-tls test-fenv-preserve test-fenv-return test-fenvinline \ > + test-fenv-tls test-fenv-preserve test-fenv-return \ > test-nearbyint-except test-fenv-clear \ > test-nearbyint-except-2 test-signgam-uchar test-signgam-uchar-init \ > test-signgam-uint test-signgam-uint-init test-signgam-ullong \ > diff --git a/math/fenv.h b/math/fenv.h > index 6cad1d3575..e6b9578d6c 100644 > --- a/math/fenv.h > +++ b/math/fenv.h > @@ -140,10 +140,6 @@ extern int fegetmode (femode_t *__modep) __THROW; > extern int fesetmode (const femode_t *__modep) __THROW; > #endif > > -/* Include optimization. */ > -#ifdef __OPTIMIZE__ > -# include <bits/fenvinline.h> > -#endif > > /* NaN support. */ > > diff --git a/math/test-fenvinline.c b/math/test-fenvinline.c > deleted file mode 100644 > index 0e5d361fff..0000000000 > --- a/math/test-fenvinline.c > +++ /dev/null > @@ -1,354 +0,0 @@ > -/* Test for fenv inline implementations. > - Copyright (C) 2015-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 _GNU_SOURCE > -# define _GNU_SOURCE > -#endif > - > -/* To make sure the fenv inline function are used. */ > -#undef __NO_MATH_INLINES > - > -#include <fenv.h> > -#include <stdio.h> > -#include <math-tests.h> > - > -/* > - Since not all architectures might define all exceptions, we define > - a private set and map accordingly. > -*/ > -#define NO_EXC 0 > -#define INEXACT_EXC 0x1 > -#define DIVBYZERO_EXC 0x2 > -#define UNDERFLOW_EXC 0x04 > -#define OVERFLOW_EXC 0x08 > -#define INVALID_EXC 0x10 > -#define ALL_EXC \ > - (INEXACT_EXC | DIVBYZERO_EXC | UNDERFLOW_EXC | OVERFLOW_EXC \ > - | INVALID_EXC) > -static int count_errors; > - > -#if FE_ALL_EXCEPT > -static void > -test_single_exception_fp_int (int exception, > - int exc_flag, > - int fe_flag, > - const char *flag_name) > -{ > - if (exception & exc_flag) > - { > - if (fetestexcept (fe_flag)) > - printf (" Pass: Exception \"%s\" is set\n", flag_name); > - else > - { > - printf (" Fail: Exception \"%s\" is not set\n", flag_name); > - ++count_errors; > - } > - } > - else > - { > - if (fetestexcept (fe_flag)) > - { > - printf (" Fail: Exception \"%s\" is set\n", flag_name); > - ++count_errors; > - } > - else > - printf (" Pass: Exception \"%s\" is not set\n", flag_name); > - } > -} > -/* Test whether a given exception was raised. */ > -static void > -test_single_exception_fp_double (int exception, > - int exc_flag, > - double fe_flag, > - const char *flag_name) > -{ > - if (exception & exc_flag) > - { > - if (fetestexcept (fe_flag)) > - printf (" Pass: Exception \"%s\" is set\n", flag_name); > - else > - { > - printf (" Fail: Exception \"%s\" is not set\n", flag_name); > - ++count_errors; > - } > - } > - else > - { > - if (fetestexcept (fe_flag)) > - { > - printf (" Fail: Exception \"%s\" is set\n", flag_name); > - ++count_errors; > - } > - else > - printf (" Pass: Exception \"%s\" is not set\n", flag_name); > - } > -} > -#endif > - > -static void > -test_exceptions (const char *test_name, int exception) > -{ > - printf ("Test: %s\n", test_name); > -#ifdef FE_DIVBYZERO > - test_single_exception_fp_double (exception, DIVBYZERO_EXC, FE_DIVBYZERO, > - "DIVBYZERO"); > -#endif > -#ifdef FE_INVALID > - test_single_exception_fp_double (exception, INVALID_EXC, FE_INVALID, > - "INVALID"); > -#endif > -#ifdef FE_INEXACT > - test_single_exception_fp_double (exception, INEXACT_EXC, FE_INEXACT, > - "INEXACT"); > -#endif > -#ifdef FE_UNDERFLOW > - test_single_exception_fp_double (exception, UNDERFLOW_EXC, FE_UNDERFLOW, > - "UNDERFLOW"); > -#endif > -#ifdef FE_OVERFLOW > - test_single_exception_fp_double (exception, OVERFLOW_EXC, FE_OVERFLOW, > - "OVERFLOW"); > -#endif > -} > - > -static void > -test_exceptionflag (void) > -{ > - printf ("Test: fegetexceptionflag (FE_ALL_EXCEPT)\n"); > -#if FE_ALL_EXCEPT > - fexcept_t excepts; > - > - feclearexcept (FE_ALL_EXCEPT); > - > - feraiseexcept (FE_INVALID); > - fegetexceptflag (&excepts, FE_ALL_EXCEPT); > - > - feclearexcept (FE_ALL_EXCEPT); > - feraiseexcept (FE_OVERFLOW | FE_INEXACT); > - > - fesetexceptflag (&excepts, FE_ALL_EXCEPT); > - > - test_single_exception_fp_int (INVALID_EXC, INVALID_EXC, FE_INVALID, > - "INVALID (int)"); > - test_single_exception_fp_int (INVALID_EXC, OVERFLOW_EXC, FE_OVERFLOW, > - "OVERFLOW (int)"); > - test_single_exception_fp_int (INVALID_EXC, INEXACT_EXC, FE_INEXACT, > - "INEXACT (int)"); > - > - /* Same test, but using double as argument */ > - feclearexcept (FE_ALL_EXCEPT); > - > - feraiseexcept (FE_INVALID); > - fegetexceptflag (&excepts, (double)FE_ALL_EXCEPT); > - > - feclearexcept (FE_ALL_EXCEPT); > - feraiseexcept (FE_OVERFLOW | FE_INEXACT); > - > - fesetexceptflag (&excepts, (double)FE_ALL_EXCEPT); > - > - test_single_exception_fp_double (INVALID_EXC, INVALID_EXC, FE_INVALID, > - "INVALID (double)"); > - test_single_exception_fp_double (INVALID_EXC, OVERFLOW_EXC, FE_OVERFLOW, > - "OVERFLOW (double)"); > - test_single_exception_fp_double (INVALID_EXC, INEXACT_EXC, FE_INEXACT, > - "INEXACT (double)"); > -#endif > -} > - > -static void > -test_fesetround (void) > -{ > -#if defined FE_TONEAREST && defined FE_TOWARDZERO > - int res1; > - int res2; > - > - printf ("Tests for fesetround\n"); > - > - /* The fesetround should not itself cause the test to fail, however it > - should either succeed for both 'int' and 'double' argument, or fail > - for both. */ > - res1 = fesetround ((int) FE_TOWARDZERO); > - res2 = fesetround ((double) FE_TOWARDZERO); > - if (res1 != res2) > - { > - printf ("fesetround (FE_TOWARDZERO) failed: %d, %d\n", res1, res2); > - ++count_errors; > - } > - > - res1 = fesetround ((int) FE_TONEAREST); > - res2 = fesetround ((double) FE_TONEAREST); > - if (res1 != res2) > - { > - printf ("fesetround (FE_TONEAREST) failed: %d, %d\n", res1, res2); > - ++count_errors; > - } > -#endif > -} > - > -#if FE_ALL_EXCEPT > -/* Tests for feenableexcept/fedisableexcept. */ > -static void > -feenable_test (const char *flag_name, fexcept_t fe_exc) > -{ > - int fe_exci = fe_exc; > - double fe_excd = fe_exc; > - int excepts; > - > - /* First disable all exceptions. */ > - if (fedisableexcept (FE_ALL_EXCEPT) == -1) > - { > - printf ("Test: fedisableexcept (FE_ALL_EXCEPT) failed\n"); > - ++count_errors; > - /* If this fails, the other tests don't make sense. */ > - return; > - } > - > - /* Test for inline macros using integer argument. */ > - excepts = feenableexcept (fe_exci); > - if (!EXCEPTION_ENABLE_SUPPORTED (fe_exci) && excepts == -1) > - { > - printf ("Test: not testing feenableexcept, it isn't implemented.\n"); > - return; > - } > - if (excepts == -1) > - { > - printf ("Test: feenableexcept (%s) failed\n", flag_name); > - ++count_errors; > - return; > - } > - if (excepts != 0) > - { > - printf ("Test: feenableexcept (%s) failed, return should be 0, is %x\n", > - flag_name, excepts); > - ++count_errors; > - } > - > - /* And now disable the exception again. */ > - excepts = fedisableexcept (fe_exc); > - if (excepts == -1) > - { > - printf ("Test: fedisableexcept (%s) failed\n", flag_name); > - ++count_errors; > - return; > - } > - if (excepts != fe_exc) > - { > - printf ("Test: fedisableexcept (%s) failed, return should be 0x%x, is 0x%x\n", > - flag_name, (unsigned int)fe_exc, excepts); > - ++count_errors; > - } > - > - /* Test for inline macros using double argument. */ > - excepts = feenableexcept (fe_excd); > - if (!EXCEPTION_ENABLE_SUPPORTED (fe_excd) && excepts == -1) > - { > - printf ("Test: not testing feenableexcept, it isn't implemented.\n"); > - return; > - } > - if (excepts == -1) > - { > - printf ("Test: feenableexcept (%s) failed\n", flag_name); > - ++count_errors; > - return; > - } > - if (excepts != 0) > - { > - printf ("Test: feenableexcept (%s) failed, return should be 0, is %x\n", > - flag_name, excepts); > - ++count_errors; > - } > - > - /* And now disable the exception again. */ > - excepts = fedisableexcept (fe_exc); > - if (excepts == -1) > - { > - printf ("Test: fedisableexcept (%s) failed\n", flag_name); > - ++count_errors; > - return; > - } > - if (excepts != fe_exc) > - { > - printf ("Test: fedisableexcept (%s) failed, return should be 0x%x, is 0x%x\n", > - flag_name, (unsigned int)fe_exc, excepts); > - ++count_errors; > - } > -} > -#endif > - > -static void > -test_feenabledisable (void) > -{ > - printf ("Tests for feenableexcepts/fedisableexcept\n"); > - > - /* We might have some exceptions still set. */ > - feclearexcept (FE_ALL_EXCEPT); > - > -#ifdef FE_DIVBYZERO > - feenable_test ("FE_DIVBYZERO", FE_DIVBYZERO); > -#endif > -#ifdef FE_INVALID > - feenable_test ("FE_INVALID", FE_INVALID); > -#endif > -#ifdef FE_INEXACT > - feenable_test ("FE_INEXACT", FE_INEXACT); > -#endif > -#ifdef FE_UNDERFLOW > - feenable_test ("FE_UNDERFLOW", FE_UNDERFLOW); > -#endif > -#ifdef FE_OVERFLOW > - feenable_test ("FE_OVERFLOW", FE_OVERFLOW); > -#endif > - fesetenv (FE_DFL_ENV); > -} > - > -static int > -do_test (void) > -{ > - /* clear all exceptions and test if all are cleared */ > - feclearexcept (FE_ALL_EXCEPT); > - test_exceptions ("feclearexcept (FE_ALL_EXCEPT) clears all exceptions", > - NO_EXC); > - > - /* raise all exceptions and test if all are raised */ > - feraiseexcept (FE_ALL_EXCEPT); > - if (EXCEPTION_TESTS (float)) > - test_exceptions ("feraiseexcept (FE_ALL_EXCEPT) raises all exceptions", > - ALL_EXC); > - > - /* Same test, but using double as argument */ > - feclearexcept ((double)FE_ALL_EXCEPT); > - test_exceptions ("feclearexcept ((double)FE_ALL_EXCEPT) clears all exceptions", > - NO_EXC); > - > - feraiseexcept ((double)FE_ALL_EXCEPT); > - if (EXCEPTION_TESTS (float)) > - test_exceptions ("feraiseexcept ((double)FE_ALL_EXCEPT) raises all exceptions", > - ALL_EXC); > - > - if (EXCEPTION_TESTS (float)) > - test_exceptionflag (); > - > - test_fesetround (); > - > - test_feenabledisable (); > - > - return count_errors; > -} > - > -#define TEST_FUNCTION do_test () > -#include "../test-skeleton.c" > diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h > deleted file mode 100644 > index f2d095a72f..0000000000 > --- a/sysdeps/powerpc/bits/fenvinline.h > +++ /dev/null > @@ -1,108 +0,0 @@ > -/* Inline floating-point environment handling functions for powerpc. > - Copyright (C) 1995-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/>. */ > - > -#if defined __GNUC__ && !defined _SOFT_FLOAT && !defined __NO_FPRS__ > - > -/* Inline definitions for fegetround. */ > -# define __fegetround_ISA300() \ > - (__extension__ ({ \ > - union { double __d; unsigned long long __ll; } __u; \ > - __asm__ __volatile__ ( \ > - ".machine push; .machine \"power9\"; mffsl %0; .machine pop" \ > - : "=f" (__u.__d)); \ > - __u.__ll & 0x0000000000000003LL; \ > - })) > - > -# define __fegetround_ISA2() \ > - (__extension__ ({ \ > - int __fegetround_result; \ > - __asm__ __volatile__ ("mcrfs 7,7 ; mfcr %0" \ > - : "=r"(__fegetround_result) : : "cr7"); \ > - __fegetround_result & 3; \ > - })) > - > -# ifdef _ARCH_PWR9 > -# define __fegetround() __fegetround_ISA300() > -# elif defined __BUILTIN_CPU_SUPPORTS__ > -# define __fegetround() \ > - (__glibc_likely (__builtin_cpu_supports ("arch_3_00")) \ > - ? __fegetround_ISA300() \ > - : __fegetround_ISA2() \ > - ) > -# else > -# define __fegetround() __fegetround_ISA2() > -# endif > - > -# define fegetround() __fegetround () > - > -# ifndef __NO_MATH_INLINES > - > -/* Builtins to mtfsb0 and mtfsb1 was introduced on GCC 9. */ > -# if !__GNUC_PREREQ(9, 0) > -/* The weird 'i#*X' constraints on the following suppress a gcc > - warning when __excepts is not a constant. Otherwise, they mean the > - same as just plain 'i'. This warning only happens in old GCC > - versions (gcc 3 or less). Otherwise plain 'i' works fine. */ > -# define __MTFSB0(__b) __asm__ __volatile__ ("mtfsb0 %0" : : "i#*X" (__b)) > -# define __MTFSB1(__b) __asm__ __volatile__ ("mtfsb1 %0" : : "i#*X" (__b)) > -# else > -# define __MTFSB0(__b) __builtin_mtfsb0 (__b) > -# define __MTFSB1(__b) __builtin_mtfsb1 (__b) > -# endif > - > -# if __GNUC_PREREQ(3, 4) > - > -#include <sys/param.h> > - > -/* Inline definition for feraiseexcept. */ > -# define feraiseexcept(__excepts) \ > - (__extension__ ({ \ > - int __e = __excepts; \ > - int __ret = 0; \ > - if (__builtin_constant_p (__e) \ > - && __builtin_popcount (__e) == 1 \ > - && __e != FE_INVALID) \ > - { \ > - __MTFSB1 ((__builtin_clz (__e))); \ > - } \ > - else \ > - __ret = feraiseexcept (__e); \ > - __ret; \ > - })) > - > -/* Inline definition for feclearexcept. */ > -# define feclearexcept(__excepts) \ > - (__extension__ ({ \ > - int __e = __excepts; \ > - int __ret = 0; \ > - if (__builtin_constant_p (__e) \ > - && __builtin_popcount (__e) == 1 \ > - && __e != FE_INVALID) \ > - { \ > - __MTFSB0 ((__builtin_clz (__e))); \ > - } \ > - else \ > - __ret = feclearexcept (__e); \ > - __ret; \ > - })) > - > -# endif /* __GNUC_PREREQ(3, 4). */ > - > -# endif /* !__NO_MATH_INLINES. */ > - > -#endif /* __GNUC__ && !_SOFT_FLOAT && !__NO_FPRS__ */ > diff --git a/sysdeps/powerpc/fpu/fegetround.c b/sysdeps/powerpc/fpu/fegetround.c > index 00b4462624..9d7762f08b 100644 > --- a/sysdeps/powerpc/fpu/fegetround.c > +++ b/sysdeps/powerpc/fpu/fegetround.c > @@ -21,10 +21,8 @@ > int > (__fegetround) (void) > { > - return __fegetround(); > + return __fegetround_inline (); > } > -#undef fegetround > -#undef __fegetround > libm_hidden_def (__fegetround) > weak_alias (__fegetround, fegetround) > libm_hidden_weak (fegetround) > diff --git a/sysdeps/powerpc/fpu/fenv_libc.h b/sysdeps/powerpc/fpu/fenv_libc.h > index e888c6621c..09dbd3e2df 100644 > --- a/sysdeps/powerpc/fpu/fenv_libc.h > +++ b/sysdeps/powerpc/fpu/fenv_libc.h > @@ -68,6 +68,14 @@ extern const fenv_t *__fe_mask_env (void) attribute_hidden; > __fr; \ > }) > > +#define __fe_mffsl() \ > + ({register fenv_union_t __fr; \ > + __asm__ __volatile__ ( \ > + ".machine push; .machine \"power9\"; mffsl %0; .machine pop" \ > + : "=f" (__fr.fenv)); \ > + __fr.l & 0x3; \ > + }) > + > #define __fe_mffscrn(rn) \ > ({register fenv_union_t __fr; \ > if (__builtin_constant_p (rn)) \ > @@ -144,6 +152,20 @@ typedef union > unsigned long long l; > } fenv_union_t; > > +static inline int > +__fegetround_inline (void) > +{ > +#ifdef _ARCH_PWR9 > + return __fe_mffsl (); > +#else > + if (__glibc_likely (GLRO(dl_hwcap2) & PPC_FEATURE2_ARCH_3_00)) > + return __fe_mffsl (); > + > + fenv_union_t fe; > + fe.fenv = fegetenv_register (); > + return fe.l & 0x3; > +#endif > +} > > static inline int > __fesetround_inline (int round) > diff --git a/sysdeps/powerpc/fpu/fraiseexcpt.c b/sysdeps/powerpc/fpu/fraiseexcpt.c > index daa13de500..1bcd31ae14 100644 > --- a/sysdeps/powerpc/fpu/fraiseexcpt.c > +++ b/sysdeps/powerpc/fpu/fraiseexcpt.c > @@ -18,7 +18,6 @@ > > #include <fenv_libc.h> > > -#undef feraiseexcept > int > __feraiseexcept (int excepts) > { > diff --git a/sysdeps/powerpc/nofpu/fraiseexcpt.c b/sysdeps/powerpc/nofpu/fraiseexcpt.c > index 2193a604e2..e60ed68048 100644 > --- a/sysdeps/powerpc/nofpu/fraiseexcpt.c > +++ b/sysdeps/powerpc/nofpu/fraiseexcpt.c > @@ -21,7 +21,6 @@ > #include "soft-supp.h" > #include <signal.h> > > -#undef feraiseexcept > int > __feraiseexcept (int x) > { >
On 3/10/20 11:51 AM, Adhemerval Zanella wrote: > > > On 09/03/2020 17:19, Paul E Murphy wrote: >> >> >> On 3/9/20 1:32 PM, Adhemerval Zanella wrote: >>> Changes from previous version: >>> >>> - Mention on commit message x86 also exports a similar optimization, >>> but on a different header. >>> >>> -- >>> >>> Similar to string2.h (18b10de7ce) and string3.h (09a596cc2c) this >>> patch removes the fenvinline.h on all architectures. Currently >>> only powerpc implements some optimizations. This kind of optimization >>> is better implemented by the compiler (which handles the architecture >>> ISA transparently). >>> >>> Also, for the specific optimized powerpc implementation the code is >>> becoming convoluted and these micro-optimization are hardly wildly >>> used, even more being a possible hotspot in realword cases >>> (non-default rounding are used only on specific cases and exception >>> handling are done most likely only on errors path). Only x86 >>> implements similar optimization (on fenv.h) also indicates that >>> these should no be on libc. >>> >>> The math/test-fenv already covers all math/test-fenvinline tests, >>> so it is safe to remove it. >>> >>> Checked on x86_64-linux-gnu and powerpc64le-linux-gnu. >>> --- >>> bits/fenvinline.h | 8 - >>> math/Makefile | 4 +- >>> math/fenv.h | 4 - >>> math/test-fenvinline.c | 354 ------------------------------ >>> sysdeps/powerpc/bits/fenvinline.h | 108 --------- >>> 5 files changed, 2 insertions(+), 476 deletions(-) >>> delete mode 100644 bits/fenvinline.h >>> delete mode 100644 math/test-fenvinline.c >>> delete mode 100644 sysdeps/powerpc/bits/fenvinline.h > > Indeed, I misread the failures on powerpc64le-linux-gnu. Below it is > an updated patch with the fegetround optimization moved to an internal > header. > diff --git a/sysdeps/powerpc/fpu/fegetround.c b/sysdeps/powerpc/fpu/fegetround.c > index 00b4462624..9d7762f08b 100644 > --- a/sysdeps/powerpc/fpu/fegetround.c > +++ b/sysdeps/powerpc/fpu/fegetround.c > @@ -21,10 +21,8 @@ > int > (__fegetround) (void) > { > - return __fegetround(); > + return __fegetround_inline (); > } > -#undef fegetround > -#undef __fegetround > libm_hidden_def (__fegetround) > weak_alias (__fegetround, fegetround) > libm_hidden_weak (fegetround) > diff --git a/sysdeps/powerpc/fpu/fenv_libc.h b/sysdeps/powerpc/fpu/fenv_libc.h > index e888c6621c..09dbd3e2df 100644 > --- a/sysdeps/powerpc/fpu/fenv_libc.h > +++ b/sysdeps/powerpc/fpu/fenv_libc.h > @@ -68,6 +68,14 @@ extern const fenv_t *__fe_mask_env (void) attribute_hidden; > __fr; \ > }) > > +#define __fe_mffsl() \ > + ({register fenv_union_t __fr; \ > + __asm__ __volatile__ ( \ > + ".machine push; .machine \"power9\"; mffsl %0; .machine pop" \ > + : "=f" (__fr.fenv)); \ > + __fr.l & 0x3; \ > + }) > + > #define __fe_mffscrn(rn) \ > ({register fenv_union_t __fr; \ > if (__builtin_constant_p (rn)) \ > @@ -144,6 +152,20 @@ typedef union > unsigned long long l; > } fenv_union_t; > > +static inline int > +__fegetround_inline (void) > +{ > +#ifdef _ARCH_PWR9 > + return __fe_mffsl (); > +#else > + if (__glibc_likely (GLRO(dl_hwcap2) & PPC_FEATURE2_ARCH_3_00)) > + return __fe_mffsl (); > + Can the above be removed, and fegetenv_register() be replaced with fegetenv_control()? Such should work optimally on all ppc machines. Otherwise, it LGTM. I have mixed feelings about regressing these inlines before compiler support arrives, but I suspect these are likely not used in performance critical places, so I am not objecting. > + fenv_union_t fe; > + fe.fenv = fegetenv_register (); > + return fe.l & 0x3; > +#endif > +} > > static inline int > __fesetround_inline (int round)
diff --git a/bits/fenvinline.h b/bits/fenvinline.h deleted file mode 100644 index 42f77b5618..0000000000 --- a/bits/fenvinline.h +++ /dev/null @@ -1,8 +0,0 @@ -/* This file provides inline versions of floating-pint environment - handling functions. If there were any. */ - -#ifndef __NO_MATH_INLINES - -/* Here is where the code would go. */ - -#endif diff --git a/math/Makefile b/math/Makefile index 84a8b94c74..f916594a51 100644 --- a/math/Makefile +++ b/math/Makefile @@ -24,7 +24,7 @@ include ../Makeconfig # Installed header files. headers := math.h bits/mathcalls.h bits/mathinline.h \ fpu_control.h complex.h bits/cmathcalls.h fenv.h \ - bits/fenv.h bits/fenvinline.h bits/mathdef.h tgmath.h \ + bits/fenv.h bits/mathdef.h tgmath.h \ bits/math-vector.h finclude/math-vector-fortran.h \ bits/libm-simd-decl-stubs.h bits/iscanonical.h \ bits/flt-eval-method.h bits/fp-fast.h bits/fp-logb.h \ @@ -233,7 +233,7 @@ tests = test-matherr-3 test-fenv basic-test \ test-misc test-fpucw test-fpucw-ieee tst-definitions test-tgmath \ test-tgmath-ret bug-nextafter bug-nexttoward bug-tgmath1 \ test-tgmath-int test-tgmath2 test-powl tst-CMPLX tst-CMPLX2 test-snan \ - test-fenv-tls test-fenv-preserve test-fenv-return test-fenvinline \ + test-fenv-tls test-fenv-preserve test-fenv-return \ test-nearbyint-except test-fenv-clear \ test-nearbyint-except-2 test-signgam-uchar test-signgam-uchar-init \ test-signgam-uint test-signgam-uint-init test-signgam-ullong \ diff --git a/math/fenv.h b/math/fenv.h index 6cad1d3575..e6b9578d6c 100644 --- a/math/fenv.h +++ b/math/fenv.h @@ -140,10 +140,6 @@ extern int fegetmode (femode_t *__modep) __THROW; extern int fesetmode (const femode_t *__modep) __THROW; #endif -/* Include optimization. */ -#ifdef __OPTIMIZE__ -# include <bits/fenvinline.h> -#endif /* NaN support. */ diff --git a/math/test-fenvinline.c b/math/test-fenvinline.c deleted file mode 100644 index 0e5d361fff..0000000000 --- a/math/test-fenvinline.c +++ /dev/null @@ -1,354 +0,0 @@ -/* Test for fenv inline implementations. - Copyright (C) 2015-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 _GNU_SOURCE -# define _GNU_SOURCE -#endif - -/* To make sure the fenv inline function are used. */ -#undef __NO_MATH_INLINES - -#include <fenv.h> -#include <stdio.h> -#include <math-tests.h> - -/* - Since not all architectures might define all exceptions, we define - a private set and map accordingly. -*/ -#define NO_EXC 0 -#define INEXACT_EXC 0x1 -#define DIVBYZERO_EXC 0x2 -#define UNDERFLOW_EXC 0x04 -#define OVERFLOW_EXC 0x08 -#define INVALID_EXC 0x10 -#define ALL_EXC \ - (INEXACT_EXC | DIVBYZERO_EXC | UNDERFLOW_EXC | OVERFLOW_EXC \ - | INVALID_EXC) -static int count_errors; - -#if FE_ALL_EXCEPT -static void -test_single_exception_fp_int (int exception, - int exc_flag, - int fe_flag, - const char *flag_name) -{ - if (exception & exc_flag) - { - if (fetestexcept (fe_flag)) - printf (" Pass: Exception \"%s\" is set\n", flag_name); - else - { - printf (" Fail: Exception \"%s\" is not set\n", flag_name); - ++count_errors; - } - } - else - { - if (fetestexcept (fe_flag)) - { - printf (" Fail: Exception \"%s\" is set\n", flag_name); - ++count_errors; - } - else - printf (" Pass: Exception \"%s\" is not set\n", flag_name); - } -} -/* Test whether a given exception was raised. */ -static void -test_single_exception_fp_double (int exception, - int exc_flag, - double fe_flag, - const char *flag_name) -{ - if (exception & exc_flag) - { - if (fetestexcept (fe_flag)) - printf (" Pass: Exception \"%s\" is set\n", flag_name); - else - { - printf (" Fail: Exception \"%s\" is not set\n", flag_name); - ++count_errors; - } - } - else - { - if (fetestexcept (fe_flag)) - { - printf (" Fail: Exception \"%s\" is set\n", flag_name); - ++count_errors; - } - else - printf (" Pass: Exception \"%s\" is not set\n", flag_name); - } -} -#endif - -static void -test_exceptions (const char *test_name, int exception) -{ - printf ("Test: %s\n", test_name); -#ifdef FE_DIVBYZERO - test_single_exception_fp_double (exception, DIVBYZERO_EXC, FE_DIVBYZERO, - "DIVBYZERO"); -#endif -#ifdef FE_INVALID - test_single_exception_fp_double (exception, INVALID_EXC, FE_INVALID, - "INVALID"); -#endif -#ifdef FE_INEXACT - test_single_exception_fp_double (exception, INEXACT_EXC, FE_INEXACT, - "INEXACT"); -#endif -#ifdef FE_UNDERFLOW - test_single_exception_fp_double (exception, UNDERFLOW_EXC, FE_UNDERFLOW, - "UNDERFLOW"); -#endif -#ifdef FE_OVERFLOW - test_single_exception_fp_double (exception, OVERFLOW_EXC, FE_OVERFLOW, - "OVERFLOW"); -#endif -} - -static void -test_exceptionflag (void) -{ - printf ("Test: fegetexceptionflag (FE_ALL_EXCEPT)\n"); -#if FE_ALL_EXCEPT - fexcept_t excepts; - - feclearexcept (FE_ALL_EXCEPT); - - feraiseexcept (FE_INVALID); - fegetexceptflag (&excepts, FE_ALL_EXCEPT); - - feclearexcept (FE_ALL_EXCEPT); - feraiseexcept (FE_OVERFLOW | FE_INEXACT); - - fesetexceptflag (&excepts, FE_ALL_EXCEPT); - - test_single_exception_fp_int (INVALID_EXC, INVALID_EXC, FE_INVALID, - "INVALID (int)"); - test_single_exception_fp_int (INVALID_EXC, OVERFLOW_EXC, FE_OVERFLOW, - "OVERFLOW (int)"); - test_single_exception_fp_int (INVALID_EXC, INEXACT_EXC, FE_INEXACT, - "INEXACT (int)"); - - /* Same test, but using double as argument */ - feclearexcept (FE_ALL_EXCEPT); - - feraiseexcept (FE_INVALID); - fegetexceptflag (&excepts, (double)FE_ALL_EXCEPT); - - feclearexcept (FE_ALL_EXCEPT); - feraiseexcept (FE_OVERFLOW | FE_INEXACT); - - fesetexceptflag (&excepts, (double)FE_ALL_EXCEPT); - - test_single_exception_fp_double (INVALID_EXC, INVALID_EXC, FE_INVALID, - "INVALID (double)"); - test_single_exception_fp_double (INVALID_EXC, OVERFLOW_EXC, FE_OVERFLOW, - "OVERFLOW (double)"); - test_single_exception_fp_double (INVALID_EXC, INEXACT_EXC, FE_INEXACT, - "INEXACT (double)"); -#endif -} - -static void -test_fesetround (void) -{ -#if defined FE_TONEAREST && defined FE_TOWARDZERO - int res1; - int res2; - - printf ("Tests for fesetround\n"); - - /* The fesetround should not itself cause the test to fail, however it - should either succeed for both 'int' and 'double' argument, or fail - for both. */ - res1 = fesetround ((int) FE_TOWARDZERO); - res2 = fesetround ((double) FE_TOWARDZERO); - if (res1 != res2) - { - printf ("fesetround (FE_TOWARDZERO) failed: %d, %d\n", res1, res2); - ++count_errors; - } - - res1 = fesetround ((int) FE_TONEAREST); - res2 = fesetround ((double) FE_TONEAREST); - if (res1 != res2) - { - printf ("fesetround (FE_TONEAREST) failed: %d, %d\n", res1, res2); - ++count_errors; - } -#endif -} - -#if FE_ALL_EXCEPT -/* Tests for feenableexcept/fedisableexcept. */ -static void -feenable_test (const char *flag_name, fexcept_t fe_exc) -{ - int fe_exci = fe_exc; - double fe_excd = fe_exc; - int excepts; - - /* First disable all exceptions. */ - if (fedisableexcept (FE_ALL_EXCEPT) == -1) - { - printf ("Test: fedisableexcept (FE_ALL_EXCEPT) failed\n"); - ++count_errors; - /* If this fails, the other tests don't make sense. */ - return; - } - - /* Test for inline macros using integer argument. */ - excepts = feenableexcept (fe_exci); - if (!EXCEPTION_ENABLE_SUPPORTED (fe_exci) && excepts == -1) - { - printf ("Test: not testing feenableexcept, it isn't implemented.\n"); - return; - } - if (excepts == -1) - { - printf ("Test: feenableexcept (%s) failed\n", flag_name); - ++count_errors; - return; - } - if (excepts != 0) - { - printf ("Test: feenableexcept (%s) failed, return should be 0, is %x\n", - flag_name, excepts); - ++count_errors; - } - - /* And now disable the exception again. */ - excepts = fedisableexcept (fe_exc); - if (excepts == -1) - { - printf ("Test: fedisableexcept (%s) failed\n", flag_name); - ++count_errors; - return; - } - if (excepts != fe_exc) - { - printf ("Test: fedisableexcept (%s) failed, return should be 0x%x, is 0x%x\n", - flag_name, (unsigned int)fe_exc, excepts); - ++count_errors; - } - - /* Test for inline macros using double argument. */ - excepts = feenableexcept (fe_excd); - if (!EXCEPTION_ENABLE_SUPPORTED (fe_excd) && excepts == -1) - { - printf ("Test: not testing feenableexcept, it isn't implemented.\n"); - return; - } - if (excepts == -1) - { - printf ("Test: feenableexcept (%s) failed\n", flag_name); - ++count_errors; - return; - } - if (excepts != 0) - { - printf ("Test: feenableexcept (%s) failed, return should be 0, is %x\n", - flag_name, excepts); - ++count_errors; - } - - /* And now disable the exception again. */ - excepts = fedisableexcept (fe_exc); - if (excepts == -1) - { - printf ("Test: fedisableexcept (%s) failed\n", flag_name); - ++count_errors; - return; - } - if (excepts != fe_exc) - { - printf ("Test: fedisableexcept (%s) failed, return should be 0x%x, is 0x%x\n", - flag_name, (unsigned int)fe_exc, excepts); - ++count_errors; - } -} -#endif - -static void -test_feenabledisable (void) -{ - printf ("Tests for feenableexcepts/fedisableexcept\n"); - - /* We might have some exceptions still set. */ - feclearexcept (FE_ALL_EXCEPT); - -#ifdef FE_DIVBYZERO - feenable_test ("FE_DIVBYZERO", FE_DIVBYZERO); -#endif -#ifdef FE_INVALID - feenable_test ("FE_INVALID", FE_INVALID); -#endif -#ifdef FE_INEXACT - feenable_test ("FE_INEXACT", FE_INEXACT); -#endif -#ifdef FE_UNDERFLOW - feenable_test ("FE_UNDERFLOW", FE_UNDERFLOW); -#endif -#ifdef FE_OVERFLOW - feenable_test ("FE_OVERFLOW", FE_OVERFLOW); -#endif - fesetenv (FE_DFL_ENV); -} - -static int -do_test (void) -{ - /* clear all exceptions and test if all are cleared */ - feclearexcept (FE_ALL_EXCEPT); - test_exceptions ("feclearexcept (FE_ALL_EXCEPT) clears all exceptions", - NO_EXC); - - /* raise all exceptions and test if all are raised */ - feraiseexcept (FE_ALL_EXCEPT); - if (EXCEPTION_TESTS (float)) - test_exceptions ("feraiseexcept (FE_ALL_EXCEPT) raises all exceptions", - ALL_EXC); - - /* Same test, but using double as argument */ - feclearexcept ((double)FE_ALL_EXCEPT); - test_exceptions ("feclearexcept ((double)FE_ALL_EXCEPT) clears all exceptions", - NO_EXC); - - feraiseexcept ((double)FE_ALL_EXCEPT); - if (EXCEPTION_TESTS (float)) - test_exceptions ("feraiseexcept ((double)FE_ALL_EXCEPT) raises all exceptions", - ALL_EXC); - - if (EXCEPTION_TESTS (float)) - test_exceptionflag (); - - test_fesetround (); - - test_feenabledisable (); - - return count_errors; -} - -#define TEST_FUNCTION do_test () -#include "../test-skeleton.c" diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h deleted file mode 100644 index f2d095a72f..0000000000 --- a/sysdeps/powerpc/bits/fenvinline.h +++ /dev/null @@ -1,108 +0,0 @@ -/* Inline floating-point environment handling functions for powerpc. - Copyright (C) 1995-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/>. */ - -#if defined __GNUC__ && !defined _SOFT_FLOAT && !defined __NO_FPRS__ - -/* Inline definitions for fegetround. */ -# define __fegetround_ISA300() \ - (__extension__ ({ \ - union { double __d; unsigned long long __ll; } __u; \ - __asm__ __volatile__ ( \ - ".machine push; .machine \"power9\"; mffsl %0; .machine pop" \ - : "=f" (__u.__d)); \ - __u.__ll & 0x0000000000000003LL; \ - })) - -# define __fegetround_ISA2() \ - (__extension__ ({ \ - int __fegetround_result; \ - __asm__ __volatile__ ("mcrfs 7,7 ; mfcr %0" \ - : "=r"(__fegetround_result) : : "cr7"); \ - __fegetround_result & 3; \ - })) - -# ifdef _ARCH_PWR9 -# define __fegetround() __fegetround_ISA300() -# elif defined __BUILTIN_CPU_SUPPORTS__ -# define __fegetround() \ - (__glibc_likely (__builtin_cpu_supports ("arch_3_00")) \ - ? __fegetround_ISA300() \ - : __fegetround_ISA2() \ - ) -# else -# define __fegetround() __fegetround_ISA2() -# endif - -# define fegetround() __fegetround () - -# ifndef __NO_MATH_INLINES - -/* Builtins to mtfsb0 and mtfsb1 was introduced on GCC 9. */ -# if !__GNUC_PREREQ(9, 0) -/* The weird 'i#*X' constraints on the following suppress a gcc - warning when __excepts is not a constant. Otherwise, they mean the - same as just plain 'i'. This warning only happens in old GCC - versions (gcc 3 or less). Otherwise plain 'i' works fine. */ -# define __MTFSB0(__b) __asm__ __volatile__ ("mtfsb0 %0" : : "i#*X" (__b)) -# define __MTFSB1(__b) __asm__ __volatile__ ("mtfsb1 %0" : : "i#*X" (__b)) -# else -# define __MTFSB0(__b) __builtin_mtfsb0 (__b) -# define __MTFSB1(__b) __builtin_mtfsb1 (__b) -# endif - -# if __GNUC_PREREQ(3, 4) - -#include <sys/param.h> - -/* Inline definition for feraiseexcept. */ -# define feraiseexcept(__excepts) \ - (__extension__ ({ \ - int __e = __excepts; \ - int __ret = 0; \ - if (__builtin_constant_p (__e) \ - && __builtin_popcount (__e) == 1 \ - && __e != FE_INVALID) \ - { \ - __MTFSB1 ((__builtin_clz (__e))); \ - } \ - else \ - __ret = feraiseexcept (__e); \ - __ret; \ - })) - -/* Inline definition for feclearexcept. */ -# define feclearexcept(__excepts) \ - (__extension__ ({ \ - int __e = __excepts; \ - int __ret = 0; \ - if (__builtin_constant_p (__e) \ - && __builtin_popcount (__e) == 1 \ - && __e != FE_INVALID) \ - { \ - __MTFSB0 ((__builtin_clz (__e))); \ - } \ - else \ - __ret = feclearexcept (__e); \ - __ret; \ - })) - -# endif /* __GNUC_PREREQ(3, 4). */ - -# endif /* !__NO_MATH_INLINES. */ - -#endif /* __GNUC__ && !_SOFT_FLOAT && !__NO_FPRS__ */