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