| Message ID | DB6PR0801MB20536A8C78D2B93DE2412C7E830C0@DB6PR0801MB2053.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] Cleanup __ieee754_sqrt(f/l) | expand |
On Wed, 20 Dec 2017, Wilco Dijkstra wrote: > Build & test OK on AArch64. I think the general design is right, but I also think this really needs a build-many-glibcs.py test run (as do subsequent patches moving calls to use sqrt/sqrtf/sqrtl directly) because of the high risk of architecture-specific breakage. (Direct calling of sqrtf128 with similar redirects to those for sqrt / sqrtf / sqrtl is also appropriate: GCC 8 has it as a built-in function so can inline calls with -fno-math-errno on powerpc64le for POWER9. So the patch should include a sqrtf128 redirect if __HAVE_DISTINCT_FLOAT128.) > diff --git a/math/w_sqrt_template.c b/math/w_sqrt_template.c > index 5fae302382d10e9b05df2665c9cb05126cd62f02..235c263e60c85422e41bca7c817148b66030438f 100644 > --- a/math/w_sqrt_template.c > +++ b/math/w_sqrt_template.c > @@ -21,6 +21,7 @@ > for each floating-point type. */ > #if __USE_WRAPPER_TEMPLATE > > +#define NO_SQRT_REDIRECT Missing preprocessor indentation, "# define".
Joseph Myers wrote: > I think the general design is right, but I also think this really needs a > build-many-glibcs.py test run (as do subsequent patches moving calls to > use sqrt/sqrtf/sqrtl directly) because of the high risk of > architecture-specific breakage. I tried, there was a failure on 32-bit x86 due to including the w_sqrt_compat.c. That's fixed now, the only failures I get are elf/check-execstack on a few targets. > (Direct calling of sqrtf128 with similar redirects to those for sqrt / > sqrtf / sqrtl is also appropriate: GCC 8 has it as a built-in function so > can inline calls with -fno-math-errno on powerpc64le for POWER9. So the > patch should include a sqrtf128 redirect if __HAVE_DISTINCT_FLOAT128.) I can't get this to work, some targets don't like __float128, others don't like _Float128 despite __HAVE_DISTINCT_FLOAT128 being defined. I don't see any calls to sqrtf128, so I think it's best to leave this for now. There are many calls to simple functions like floor, round etc that aren't currently inlined at all, so inlining those seems like a more useful thing to do right now. I've renamed the define to NO_MATH_REDIRECT to make this simple. Latest version: This patch series cleans up the many uses of __ieee754_sqrt(f/l) in GLIBC. The goal is to enable GCC to do the inlining, and if this fails call the __ieee754_sqrt function. This is done by internally declaring sqrt with asm redirects. The compat symbols and sqrt wrappers need to disable the redirect. The redirect is also disabled if there are already redirects defined when using -ffinite-math-only. All math functions (but not math tests) are built with -fno-math-errno which means GCC will typically inline sqrt as a single instruction. This means targets are no longer forced to add a special inline for sqrt. Passes buildmanyglibcs. ChangeLog: 2017-12-21 Wilco Dijkstra <wdijkstr@arm.com> * include/math.h (sqrt): Declare with asm redirect. (sqrtf): Likewise. (sqrtl): Likewise. * Makeconfig: Add -fno-math-errno for libc/libm, but build tests with -fmath-errno. * math/w_sqrt_compat.c: Define NO_MATH_REDIRECT. * math/w_sqrt_template.c: Likewise. * math/w_sqrtf_compat.c: Likewise. * math/w_sqrtl_compat.c: Likewise. * sysdeps/i386/fpu/w_sqrt.c: Likewise. * sysdeps/i386/fpu/w_sqrt_compat.c: Likewise. -- diff --git a/Makeconfig b/Makeconfig index 34bed9790fac1fd0be9e16b7fe2fa6f5c479b32b..b9da1c137bc495a2f63064765225dc8878cde306 100644 --- a/Makeconfig +++ b/Makeconfig @@ -831,6 +831,9 @@ endif # disable any optimization that assume default rounding mode. +math-flags = -frounding-math +# Build libc/libm using -fno-math-errno, but run testsuite with -fmath-errno. ++extra-math-flags = $(if $(filter nonlib testsuite,$(in-module)),-fmath-errno,-fno-math-errno) + # We might want to compile with some stack-protection flag. ifneq ($(stack-protector),) +stack-protector=$(stack-protector) @@ -966,6 +969,7 @@ endif override CFLAGS = -std=gnu11 -fgnu89-inline $(config-extra-cflags) \ $(filter-out %frame-pointer,$(+cflags)) $(+gccwarn-c) \ + $(+extra-math-flags) \ $(sysdep-CFLAGS) $(CFLAGS-$(suffix $@)) $(CFLAGS-$(<F)) \ $(CFLAGS-$(@F)) $(tls-model) \ $(foreach lib,$(libof-$(basename $(@F))) \ diff --git a/include/math.h b/include/math.h index 7ee291fc972088a1409949326f7024e8e3fe5415..adac78f497b0787757c20c4b9ba52fc9e43a7629 100644 --- a/include/math.h +++ b/include/math.h @@ -54,5 +54,17 @@ libm_hidden_proto (__expf128) libm_hidden_proto (__expm1f128) # endif +# if !(defined __FINITE_MATH_ONLY__ && __FINITE_MATH_ONLY__ > 0) +# ifndef NO_MATH_REDIRECT +/* Declare sqrt for use within GLIBC. Sqrt will typically inline into a + single instruction. Use an asm to avoid use of PLTs if it doesn't. */ +float (sqrtf) (float) asm ("__ieee754_sqrtf"); +double (sqrt) (double) asm ("__ieee754_sqrt"); +# ifndef __NO_LONG_DOUBLE_MATH +long double (sqrtl) (long double) asm ("__ieee754_sqrtl"); +# endif +# endif +# endif + #endif #endif diff --git a/math/w_sqrt_compat.c b/math/w_sqrt_compat.c index 3280d2fbb86af2aeaf6e686fd38579b209c901aa..1a8db2fdef87e57df2f7dda117ef3ee48581ca1d 100644 --- a/math/w_sqrt_compat.c +++ b/math/w_sqrt_compat.c @@ -16,6 +16,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#define NO_MATH_REDIRECT #include <math.h> #include <math_private.h> #include <math-svid-compat.h> diff --git a/math/w_sqrt_template.c b/math/w_sqrt_template.c index 5fae302382d10e9b05df2665c9cb05126cd62f02..f03f04751a9334fc7cdb8c954fbbcd845ac8670f 100644 --- a/math/w_sqrt_template.c +++ b/math/w_sqrt_template.c @@ -21,6 +21,7 @@ for each floating-point type. */ #if __USE_WRAPPER_TEMPLATE +# define NO_MATH_REDIRECT # include <errno.h> # include <fenv.h> # include <math.h> diff --git a/math/w_sqrtf_compat.c b/math/w_sqrtf_compat.c index 6c8c7e3857c7dacf52de6b575a2386095688b5dc..fe7418dbdcdbdb571cb1b9743cf9b17eaa5c8cb2 100644 --- a/math/w_sqrtf_compat.c +++ b/math/w_sqrtf_compat.c @@ -16,6 +16,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#define NO_MATH_REDIRECT #include <math.h> #include <math_private.h> #include <math-svid-compat.h> diff --git a/math/w_sqrtl_compat.c b/math/w_sqrtl_compat.c index 0590f6d155fee27d41bfd42c1dbdf19c381ba1c8..3fda9cf991e430f259b9d26f15219181b3e6dcbb 100644 --- a/math/w_sqrtl_compat.c +++ b/math/w_sqrtl_compat.c @@ -16,6 +16,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#define NO_MATH_REDIRECT #include <math.h> #include <math_private.h> #include <math-svid-compat.h> diff --git a/sysdeps/i386/fpu/w_sqrt.c b/sysdeps/i386/fpu/w_sqrt.c index d37a5d55bf439cebdff05f05dd66f910d6d48c18..8bef04e68a7be3d21d89e6eb41dcfa2561f3f6a5 100644 --- a/sysdeps/i386/fpu/w_sqrt.c +++ b/sysdeps/i386/fpu/w_sqrt.c @@ -1,5 +1,6 @@ /* The inline __ieee754_sqrt is not correctly rounding; it's OK for most internal uses in glibc, but not for sqrt itself. */ +#define NO_MATH_REDIRECT #define __ieee754_sqrt __avoid_ieee754_sqrt #include <math.h> #include <math_private.h> diff --git a/sysdeps/i386/fpu/w_sqrt_compat.c b/sysdeps/i386/fpu/w_sqrt_compat.c index ddd36d0964c2945579bc59b02f127af88384acca..dd485f4b88c7f5348abaab81a1308aeadc9594ec 100644 --- a/sysdeps/i386/fpu/w_sqrt_compat.c +++ b/sysdeps/i386/fpu/w_sqrt_compat.c @@ -1,5 +1,6 @@ /* The inline __ieee754_sqrt is not correctly rounding; it's OK for most internal uses in glibc, but not for sqrt itself. */ +#define NO_MATH_REDIRECT #define __ieee754_sqrt __avoid_ieee754_sqrt #include <math.h> #include <math_private.h>
On Thu, 21 Dec 2017, Wilco Dijkstra wrote: > > (Direct calling of sqrtf128 with similar redirects to those for sqrt / > > sqrtf / sqrtl is also appropriate: GCC 8 has it as a built-in function so > > can inline calls with -fno-math-errno on powerpc64le for POWER9. So the > > patch should include a sqrtf128 redirect if __HAVE_DISTINCT_FLOAT128.) > > I can't get this to work, some targets don't like __float128, others > don't like _Float128 despite __HAVE_DISTINCT_FLOAT128 being defined. I It's not being defined, it's being 1 rather than 0 (#if, not #ifdef). You should use _Float128, not __float128; bits/floatn.h handles defining _Float128 as a typedef if necessary for older compilers. > don't see any calls to sqrtf128, so I think it's best to leave this for > now. It's used (more precisely, __ieee754_sqrtf128 is used, which ought to change to calling sqrtf128 to allow inlining on POWER9 with GCC 8, just as with the changes to other __ieee754_sqrt* calls) (a) via the #defines in sysdeps/ieee754/float128/float128_private.h before ldbl-128 files are included #define __ieee754_sqrtl __ieee754_sqrtf128 #define __sqrtl __sqrtf128 #define sqrtl sqrtf128 (which I expect won't need changing in the __ieee754_sqrt* -> sqrt* changes, since they already handle all variants of the function name) and (b) via sysdeps/generic/math-type-macros.h defining #define M_SQRT M_SUF (__ieee754_sqrt) which is then used in various type-generic templates, which thus end up using __ieee754_sqrtf128 when built for float128 (and changing that M_SQRT definition will obviously result in all types, including float128, changing at the same time to use sqrt* instead of __ieee754_sqrt*). > There are many calls to simple functions like floor, round etc that > aren't currently inlined at all, so inlining those seems like a more > useful thing to do right now. I've renamed the define to > NO_MATH_REDIRECT to make this simple. All those functions will of course need to handle float128 versions exactly the same. (The GCC patch to support floorf128 etc. as built-in functions is still pending review, but doing the glibc changes does not in any way depend on such GCC changes, it simply means that such GCC changes will result in better optimization in glibc.)
Joseph Myers wrote: > It's not being defined, it's being 1 rather than 0 (#if, not #ifdef). > You should use _Float128, not __float128; bits/floatn.h handles defining > _Float128 as a typedef if necessary for older compilers. OK I can get that to work, but I still get odd issues with sqrtf128 being called from the testsuite, while libm.so only defines __sqrtf128, so it gives undefined symbol errors on x64. So I wonder whether in external headers we should never do this: #define sqrtl sqrtf128 Unless there is an easy fix for these issues, I can't see any point in including it in my current patches - improving float/double inlining in this release gives much more benefit to all targets. Wilco
On Thu, 21 Dec 2017, Wilco Dijkstra wrote: > Joseph Myers wrote: > > > It's not being defined, it's being 1 rather than 0 (#if, not #ifdef). > > You should use _Float128, not __float128; bits/floatn.h handles defining > > _Float128 as a typedef if necessary for older compilers. > > OK I can get that to work, but I still get odd issues with sqrtf128 being > called from the testsuite, while libm.so only defines __sqrtf128, so it > gives undefined symbol errors on x64. So I wonder whether in external > headers we should never do this: > > #define sqrtl sqrtf128 float128_private.h isn't an external header. It includes math.h and other headers, then redefines symbols from them, and is included before the main implementation of a float128 function. Naturally your problem needs debugging, since of course libm should be exporting sqrtf128. (And float128_private.h shouldn't be included at all when the type-generic wrappers are, so shouldn't have any opportunity to cause trouble with the automatically generated w_sqrtf128 which is what ought to be defining sqrtf128 as an alias to __sqrtf128.) > Unless there is an easy fix for these issues, I can't see any point in > including it in my current patches - improving float/double inlining in this > release gives much more benefit to all targets. Using sqrt consistently for float / double means changing the M_SQRT definition, which means sqrtf128 gets used as well, which means you need to do something for sqrtf128 simply to avoid localplt failures on non-POWER9 systems with distinct float128, which clearly indicates handling all types consistently.
Joseph Myers wrote: > float128_private.h isn't an external header. It includes math.h and other > headers, then redefines symbols from them, and is included before the main > implementation of a float128 function. Naturally your problem needs > debugging, since of course libm should be exporting sqrtf128. (And > float128_private.h shouldn't be included at all when the type-generic > wrappers are, so shouldn't have any opportunity to cause trouble with the > automatically generated w_sqrtf128 which is what ought to be defining > sqrtf128 as an alias to __sqrtf128.) Well it seems the issue is all the autogenerated magic templates. It appears they are used to both define simple wrappers as well as complex float128 functions which happen to use sqrt (in the form of M_SQRT). For the former you want to disable the asm redirect, for the latter you must redirect it. Given they are all generated in the same way, it's impossible to do the right thing. So I don't think we can change M_SQRT until the wrapper magic has been removed or at least greatly simplified. Here is the latest version, which adds redirect for float128 but keeps M_SQRT as is - this passes on x86, x64, ppc, aarch64: This patch series cleans up the many uses of __ieee754_sqrt(f/l) in GLIBC. The goal is to enable GCC to do the inlining, and if this fails call the __ieee754_sqrt function. This is done by internally declaring sqrt with asm redirects. The compat symbols and sqrt wrappers need to disable the redirect. The redirect is also disabled if there are already redirects defined when using -ffinite-math-only. This means targets are no longer forced to add a special inline for sqrt. All math functions (but not math tests) are built with -fno-math-errno which means GCC will typically inline sqrt as a single instruction. ChangeLog: 2017-12-22 Wilco Dijkstra <wdijkstr@arm.com> * math/Makefile: Emit NO_MATH_REDIRECT in wrapper templates. * include/math.h (sqrt): Declare with asm redirect. (sqrtf): Likewise. (sqrtl): Likewise. * Makeconfig: Add -fno-math-errno for libc/libm, but build tests with -fmath-errno. * math/w_sqrt_compat.c: Define NO_MATH_REDIRECT. * math/w_sqrt_template.c: Likewise. * math/w_sqrtf_compat.c: Likewise. * math/w_sqrtl_compat.c: Likewise. * sysdeps/i386/fpu/w_sqrt.c: Likewise. * sysdeps/i386/fpu/w_sqrt_compat.c: Likewise. -- diff --git a/Makeconfig b/Makeconfig index 34bed9790fac1fd0be9e16b7fe2fa6f5c479b32b..b9da1c137bc495a2f63064765225dc8878cde306 100644 --- a/Makeconfig +++ b/Makeconfig @@ -831,6 +831,9 @@ endif # disable any optimization that assume default rounding mode. +math-flags = -frounding-math +# Build libc/libm using -fno-math-errno, but run testsuite with -fmath-errno. ++extra-math-flags = $(if $(filter nonlib testsuite,$(in-module)),-fmath-errno,-fno-math-errno) + # We might want to compile with some stack-protection flag. ifneq ($(stack-protector),) +stack-protector=$(stack-protector) @@ -966,6 +969,7 @@ endif override CFLAGS = -std=gnu11 -fgnu89-inline $(config-extra-cflags) \ $(filter-out %frame-pointer,$(+cflags)) $(+gccwarn-c) \ + $(+extra-math-flags) \ $(sysdep-CFLAGS) $(CFLAGS-$(suffix $@)) $(CFLAGS-$(<F)) \ $(CFLAGS-$(@F)) $(tls-model) \ $(foreach lib,$(libof-$(basename $(@F))) \ diff --git a/include/math.h b/include/math.h index 7ee291fc972088a1409949326f7024e8e3fe5415..c25033fcd5add2f4067fdbb51fb345056d6a1400 100644 --- a/include/math.h +++ b/include/math.h @@ -54,5 +54,20 @@ libm_hidden_proto (__expf128) libm_hidden_proto (__expm1f128) # endif +# if !(defined __FINITE_MATH_ONLY__ && __FINITE_MATH_ONLY__ > 0) +# ifndef NO_MATH_REDIRECT +/* Declare sqrt for use within GLIBC. Sqrt will typically inline into a + single instruction. Use an asm to avoid use of PLTs if it doesn't. */ +float (sqrtf) (float) asm ("__ieee754_sqrtf"); +double (sqrt) (double) asm ("__ieee754_sqrt"); +# ifndef __NO_LONG_DOUBLE_MATH +long double (sqrtl) (long double) asm ("__ieee754_sqrtl"); +# endif +# if __HAVE_DISTINCT_FLOAT128 > 0 +_Float128 (sqrtf128) (_Float128) asm ("__ieee754_sqrtf128"); +# endif +# endif +# endif + #endif #endif diff --git a/math/Makefile b/math/Makefile index 8978f2ef963e67acb4e3d252733cdb0e19cf6921..f9b083d8c4049bdfdaaff0a78aa26139ace83bf5 100644 --- a/math/Makefile +++ b/math/Makefile @@ -397,6 +397,7 @@ $(objpfx)gen-libm-templates.stmp: Makefile type=$${type%__*}; \ file=$(objpfx)$${gcall%F*}$${suff}$${gcall#*F}.c; \ ( \ + echo "#define NO_MATH_REDIRECT"; \ echo "#include <math-type-macros-$${type}.h>"; \ echo "#include <$${func}_template.c>"; \ ) > $${file}; \ diff --git a/math/w_sqrt_compat.c b/math/w_sqrt_compat.c index 3280d2fbb86af2aeaf6e686fd38579b209c901aa..1a8db2fdef87e57df2f7dda117ef3ee48581ca1d 100644 --- a/math/w_sqrt_compat.c +++ b/math/w_sqrt_compat.c @@ -16,6 +16,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#define NO_MATH_REDIRECT #include <math.h> #include <math_private.h> #include <math-svid-compat.h> diff --git a/math/w_sqrt_template.c b/math/w_sqrt_template.c index 5fae302382d10e9b05df2665c9cb05126cd62f02..f03f04751a9334fc7cdb8c954fbbcd845ac8670f 100644 --- a/math/w_sqrt_template.c +++ b/math/w_sqrt_template.c @@ -21,6 +21,7 @@ for each floating-point type. */ #if __USE_WRAPPER_TEMPLATE +# define NO_MATH_REDIRECT # include <errno.h> # include <fenv.h> # include <math.h> diff --git a/math/w_sqrtf_compat.c b/math/w_sqrtf_compat.c index 6c8c7e3857c7dacf52de6b575a2386095688b5dc..fe7418dbdcdbdb571cb1b9743cf9b17eaa5c8cb2 100644 --- a/math/w_sqrtf_compat.c +++ b/math/w_sqrtf_compat.c @@ -16,6 +16,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#define NO_MATH_REDIRECT #include <math.h> #include <math_private.h> #include <math-svid-compat.h> diff --git a/math/w_sqrtl_compat.c b/math/w_sqrtl_compat.c index 0590f6d155fee27d41bfd42c1dbdf19c381ba1c8..3fda9cf991e430f259b9d26f15219181b3e6dcbb 100644 --- a/math/w_sqrtl_compat.c +++ b/math/w_sqrtl_compat.c @@ -16,6 +16,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#define NO_MATH_REDIRECT #include <math.h> #include <math_private.h> #include <math-svid-compat.h> diff --git a/sysdeps/i386/fpu/w_sqrt.c b/sysdeps/i386/fpu/w_sqrt.c index d37a5d55bf439cebdff05f05dd66f910d6d48c18..8bef04e68a7be3d21d89e6eb41dcfa2561f3f6a5 100644 --- a/sysdeps/i386/fpu/w_sqrt.c +++ b/sysdeps/i386/fpu/w_sqrt.c @@ -1,5 +1,6 @@ /* The inline __ieee754_sqrt is not correctly rounding; it's OK for most internal uses in glibc, but not for sqrt itself. */ +#define NO_MATH_REDIRECT #define __ieee754_sqrt __avoid_ieee754_sqrt #include <math.h> #include <math_private.h> diff --git a/sysdeps/i386/fpu/w_sqrt_compat.c b/sysdeps/i386/fpu/w_sqrt_compat.c index ddd36d0964c2945579bc59b02f127af88384acca..dd485f4b88c7f5348abaab81a1308aeadc9594ec 100644 --- a/sysdeps/i386/fpu/w_sqrt_compat.c +++ b/sysdeps/i386/fpu/w_sqrt_compat.c @@ -1,5 +1,6 @@ /* The inline __ieee754_sqrt is not correctly rounding; it's OK for most internal uses in glibc, but not for sqrt itself. */ +#define NO_MATH_REDIRECT #define __ieee754_sqrt __avoid_ieee754_sqrt #include <math.h> #include <math_private.h>
On Fri, 22 Dec 2017, Wilco Dijkstra wrote: > Joseph Myers wrote: > > > float128_private.h isn't an external header. It includes math.h and other > > headers, then redefines symbols from them, and is included before the main > > implementation of a float128 function. Naturally your problem needs > > debugging, since of course libm should be exporting sqrtf128. (And > > float128_private.h shouldn't be included at all when the type-generic > > wrappers are, so shouldn't have any opportunity to cause trouble with the > > automatically generated w_sqrtf128 which is what ought to be defining > > sqrtf128 as an alias to __sqrtf128.) > > Well it seems the issue is all the autogenerated magic templates. It appears > they are used to both define simple wrappers as well as complex float128 > functions which happen to use sqrt (in the form of M_SQRT). For the former > you want to disable the asm redirect, for the latter you must redirect it. Given > they are all generated in the same way, it's impossible to do the right thing. > So I don't think we can change M_SQRT until the wrapper magic has been > removed or at least greatly simplified. "impossible to do the right thing" does not make sense here. Whether redirections should be disabled is a function of the particular template in question, and so should be handled through the templates that want to disable redirection defining NO_MATH_REDIRECT and those that don't not defining it. Defining it globally in the Makefile as this patch version does is obviously wrong. Is your problem actually that math-type-macros-float128.h, unlike the other math-type-macros-*.h files, includes <math.h> and <complex.h>, meaning they get included before any code from the template has a chance to define NO_MATH_REDIRECT? If so, I'd suggest removing those includes and seeing if that helps. Now, maybe it's not enough in all cases: math-type-macros-double.h includes libm-alias-double.h, of which the ldbl-opt version includes math_ldbl_opt.h which includes math.h, and likewise for ldouble. So it would also be important to look at sqrt / sqrtl wrappers in *static* libm for ldbl-opt configurations (as for most configurations, only static libm is using the type-generic wrappers rather than the compat ones - but we don't have good testsuite coverage for whether static libraries provide the intended API, so missing symbols in them could easily not result in any test failures). I'm not sure anything ought to be depending on math_ldbl_opt.h including math.h and math_private.h either; quite possibly those includes could also be safely removed, maybe adding explicit includes to any source files that were relying on them (though at that point I think you're in the territory of wanting before-and-after comparison of stripped installed shared libraries from build-many-glibcs.py --strip, to make sure there are no changes to installed libraries at all). But while I think such include removals are a sensible cleanup, relying on not having any indirect inclusion path to math.h from math-type-macros-*.h is also fragile, and I don't think it's necessary. Rather, you could have a mechanism for templates to cause NO_MATH_REDIRECT to be defined before the automatic inclusion of math-type-macros-*.h. For example, have the math/Makefile template logic generate such a define for functions in a makefile variable $(gen-calls-no-math-redirect). If you do something like that, you don't need to remove any includes. And you also avoid the clear incorrectness of defining NO_MATH_REDIRECT for all templates rather than only particular cases needing it. (Once you have such makefile logic of course you no longer define NO_MATH_REDIRECT directly in w_sqrt_template.c.)
Another observation on the use of -fno-math-errno for building libm:
ldbl-opt configurations build libnldbl_nonshared.a, a library of wrappers
such as nldbl-sqrt.c:
#include "nldbl-compat.h"
double
attribute_hidden
sqrtl (double x)
{
return sqrt (x);
}
These are intended for use with compilers that are (a) using 64-bit long
double in such a configuration and (b) do not support asm redirection, so
that a call to sqrtl actually ends up calling sqrtl in the object file and
so needs such a wrapper to end up calling the double function (whereas
with asm redirection support, math.h arranges for long double function
calls to be redirected to call the double functions). Thus, this sqrtl
wrapper needs to call the public sqrt function, both without inlining that
assumes no-math-errno and without any redirection of either sqrtl or sqrt
to corresponding __ieee754_* names. So it needs to disable redirection,
*and* the whole of libnldbl_nonshared.a needs to be built with
-fmath-errno because it's meant to be calling public errno-setting
functions. And when other libm functions such as floor get redirection,
the libnldbl_nonshared.a versions of those need to disable it as well (not
because of any errno setting issues, but because nldbl-floor.c needs to
provide the public symbol floorl, and to call the symbol floor, if not
inlined, rather than the symbol __floor which is not exported from
libm.so).
(It may be that we should declare support for asm redirection a required
feature for compilers used with installed glibc headers, which would both
allow some header cleanup and allow removing libnldbl_nonshared.a - I
think it makes sense to have an official list of required compiler
features, beyond C90/C++98, and to include this on that list, and suspect
that actually compilers without asm redirection support already don't work
with installed headers, generally or in this particular case. But while
we haven't declared that a required feature, certain libm changes need to
allow for libnldbl_nonshared.a.)
diff --git a/Makeconfig b/Makeconfig index 34bed9790fac1fd0be9e16b7fe2fa6f5c479b32b..b9da1c137bc495a2f63064765225dc8878cde306 100644 --- a/Makeconfig +++ b/Makeconfig @@ -831,6 +831,9 @@ endif # disable any optimization that assume default rounding mode. +math-flags = -frounding-math +# Build libc/libm using -fno-math-errno, but run testsuite with -fmath-errno. ++extra-math-flags = $(if $(filter nonlib testsuite,$(in-module)),-fmath-errno,-fno-math-errno) + # We might want to compile with some stack-protection flag. ifneq ($(stack-protector),) +stack-protector=$(stack-protector) @@ -966,6 +969,7 @@ endif override CFLAGS = -std=gnu11 -fgnu89-inline $(config-extra-cflags) \ $(filter-out %frame-pointer,$(+cflags)) $(+gccwarn-c) \ + $(+extra-math-flags) \ $(sysdep-CFLAGS) $(CFLAGS-$(suffix $@)) $(CFLAGS-$(<F)) \ $(CFLAGS-$(@F)) $(tls-model) \ $(foreach lib,$(libof-$(basename $(@F))) \ diff --git a/include/math.h b/include/math.h index 7ee291fc972088a1409949326f7024e8e3fe5415..f047308c4b1dc6099bcf56389649509b63b28996 100644 --- a/include/math.h +++ b/include/math.h @@ -54,5 +54,17 @@ libm_hidden_proto (__expf128) libm_hidden_proto (__expm1f128) # endif +# if !(defined __FINITE_MATH_ONLY__ && __FINITE_MATH_ONLY__ > 0) +# ifndef NO_SQRT_REDIRECT +/* Declare sqrt for use within GLIBC. Sqrt will typically inline into a + single instruction. Use an asm to avoid use of PLTs if it doesn't. */ +float (sqrtf) (float) asm ("__ieee754_sqrtf"); +double (sqrt) (double) asm ("__ieee754_sqrt"); +# ifndef __NO_LONG_DOUBLE_MATH +long double (sqrtl) (long double) asm ("__ieee754_sqrtl"); +# endif +# endif +# endif + #endif #endif diff --git a/math/w_sqrt_compat.c b/math/w_sqrt_compat.c index 3280d2fbb86af2aeaf6e686fd38579b209c901aa..fe068af9597ffb0f15b32cd454b4c34ba8bc060e 100644 --- a/math/w_sqrt_compat.c +++ b/math/w_sqrt_compat.c @@ -16,6 +16,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#define NO_SQRT_REDIRECT #include <math.h> #include <math_private.h> #include <math-svid-compat.h> diff --git a/math/w_sqrt_template.c b/math/w_sqrt_template.c index 5fae302382d10e9b05df2665c9cb05126cd62f02..235c263e60c85422e41bca7c817148b66030438f 100644 --- a/math/w_sqrt_template.c +++ b/math/w_sqrt_template.c @@ -21,6 +21,7 @@ for each floating-point type. */ #if __USE_WRAPPER_TEMPLATE +#define NO_SQRT_REDIRECT # include <errno.h> # include <fenv.h> # include <math.h> diff --git a/math/w_sqrtf_compat.c b/math/w_sqrtf_compat.c index 6c8c7e3857c7dacf52de6b575a2386095688b5dc..880be0936d9a20f9aa75cf2d66000f0c621f090f 100644 --- a/math/w_sqrtf_compat.c +++ b/math/w_sqrtf_compat.c @@ -16,6 +16,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#define NO_SQRT_REDIRECT #include <math.h> #include <math_private.h> #include <math-svid-compat.h> diff --git a/math/w_sqrtl_compat.c b/math/w_sqrtl_compat.c index 0590f6d155fee27d41bfd42c1dbdf19c381ba1c8..bff77fd31bf2c62394e0d54e6b2e7bc9296226d6 100644 --- a/math/w_sqrtl_compat.c +++ b/math/w_sqrtl_compat.c @@ -16,6 +16,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#define NO_SQRT_REDIRECT #include <math.h> #include <math_private.h> #include <math-svid-compat.h>
Here is the updated version of the sqrt cleanup. Makefile magic is now fixed to do a single selection at the top level (I couldn't add this to math-flags as somehow that doesn't work properly). This patch series cleans up the many uses of __ieee754_sqrt(f/l) in GLIBC. The goal is to enable GCC to do the inlining, and if this fails call the __ieee754_sqrt function. This is done by internally declaring sqrt with asm redirects. The compat symbols and sqrt wrappers need to disable the redirect. The redirect is also disabled if there are already redirects defined when using -ffinite-math-only. All math functions (but not math tests) are built with -fno-math-errno which means GCC will typically inline sqrt as a single instruction. This means targets are no longer forced to add a special inline for sqrt. Build & test OK on AArch64. ChangeLog: 2017-12-20 Wilco Dijkstra <wdijkstr@arm.com> * include/math.h (sqrt): Declare with asm redirect. (sqrtf): Likewise. (sqrtl): Likewise. * Makeconfig: Add -fno-math-errno for libc/libm, but build tests with -fmath-errno. * math/w_sqrt_compat.c: Define NO_SQRT_REDIRECT. * math/w_sqrt_template.c: Likewise. * math/w_sqrtf_compat.c: Likewise. * math/w_sqrtl_compat.c: Likewise. --