[v2,1/3] Cleanup __ieee754_sqrt(f/l)
Commit Message
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.
--
Comments
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.
@@ -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))) \
@@ -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
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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>