[AArch64] Add inlines for signbit (v2)
Commit Message
> Joseph Myers wrote:
> On Mon, 27 Apr 2015, Wilco Dijkstra wrote:
>
> > > Joseph Myers wrote:
> > > On Wed, 15 Apr 2015, Wilco Dijkstra wrote:
> > >
> > > > Is there a reason this couldn't be done by default in math.h similar to isgreater
> (unlike
> > > > __builtin_isinf et al, GCC implements signbit efficiently and correctly).
> > >
> > > It is, however, not type-generic in GCC
> > > <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36757>, so you'd still need
> > > to have a macro expansion checking sizeof.
> >
> > Yes indeed. However the issue is how to deal with the targets that currently
> > define inlines. Would it be reasonable just to leave them and add a signbit
> > expansion for GCC >= 3.0 that directly uses the builtins?
> > (if we define __signbit(f/l) to be __builtin_signbit(f/l), things may go
> > wrong as the various mathinlines don't do an undef first).
>
> I'd think defining __signbit to __builtin_signbit, etc., would be
> appropriate (with conditionals in the mathinline.h files to disable their
> local definitions for GCC versions where __builtin_signbit works and is at
> least as good for that architecture). It would also cover cases within
> glibc that directly use the __signbit etc. names (although arguably those
> should move to using the type-generic macros; likewise direct uses of e.g.
> __finite, __isnan).
OK, I tried that and it works fine without having to change all the targets if
I define __signbit at the end of math.h.
Add inlining of the __signbit(f/l) functions for all targets using the GCC
builtin functions when available. The out-of-line implementations need undefs
to ensure the correct symbols are exported.
OK for commit?
ChangeLog:
2015-05-22 Wilco Dijkstra <wdijkstr@arm.com>
* math/math.h: (__signbit): Define. (__signbitf): Define.
(__signbitl): Define.
* sysdeps/ieee754/dbl-64/s_signbit.c: Undef __signbit.
* sysdeps/ieee754/flt-32/s_signbitf.c: Undef __signbitf.
* sysdeps/ieee754/ldbl-128/s_signbitl.c: Undef __signbitl.
* sysdeps/ieee754/ldbl-128ibm/s_signbitl.c: Likewise.
* sysdeps/ieee754/ldbl-64-128/s_signbitl.c: Likewise.
* sysdeps/ieee754/ldbl-96/s_signbitl.c: Likewise.
---
math/math.h | 7 +++++++
sysdeps/ieee754/dbl-64/s_signbit.c | 3 ++-
sysdeps/ieee754/flt-32/s_signbitf.c | 3 ++-
sysdeps/ieee754/ldbl-128/s_signbitl.c | 3 ++-
sysdeps/ieee754/ldbl-128ibm/s_signbitl.c | 2 ++
sysdeps/ieee754/ldbl-64-128/s_signbitl.c | 1 +
sysdeps/ieee754/ldbl-96/s_signbitl.c | 3 ++-
7 files changed, 18 insertions(+), 4 deletions(-)
Comments
On Fri, 22 May 2015, Wilco Dijkstra wrote:
> OK, I tried that and it works fine without having to change all the targets if
> I define __signbit at the end of math.h.
If a target's inline __signbit definitions are completely obsolete with
this change, they should be removed as part of it. That means at least
removing sysdeps/tile/bits/mathinline.h, since it only uses
__builtin_signbit* and GCC versions before 4.0 don't support Tile.
(ColdFire bits/mathinline.h confuses things by not having a GCC version
conditional and so potentially using the builtins with GCC versions before
4.0 that had ColdFire support. But since 3.4 didn't support
__builtin_signbit, I think that can be considered a clear bug and so the
patch should also remove sysdeps/m68k/coldfire/fpu/bits/mathinline.h as
obsoleted by the generic changes.)
> +# if __GNUC_PREREQ (4,0)
> +# undef __signbitl
Why the #undef? I don't see any headers with macro definitions of
__signbitl that need to be removed in math.h.
> Joseph wrote:
> On Fri, 22 May 2015, Wilco Dijkstra wrote:
>
> > OK, I tried that and it works fine without having to change all the targets if
> > I define __signbit at the end of math.h.
>
> If a target's inline __signbit definitions are completely obsolete with
> this change, they should be removed as part of it. That means at least
> removing sysdeps/tile/bits/mathinline.h, since it only uses
> __builtin_signbit* and GCC versions before 4.0 don't support Tile.
> (ColdFire bits/mathinline.h confuses things by not having a GCC version
> conditional and so potentially using the builtins with GCC versions before
> 4.0 that had ColdFire support. But since 3.4 didn't support
> __builtin_signbit, I think that can be considered a clear bug and so the
> patch should also remove sysdeps/m68k/coldfire/fpu/bits/mathinline.h as
> obsoleted by the generic changes.)
OK, I'll update the patch to remove those.
> > +# if __GNUC_PREREQ (4,0)
> > +# undef __signbitl
>
> Why the #undef? I don't see any headers with macro definitions of
> __signbitl that need to be removed in math.h.
Unless nldbl support is dead we need it as sysdeps/ldbl-opt/nldbl-signbit.c does:
#define __signbitl __signbitl_XXX
#include "nldbl-compat.h"
nldbl-compat.h includes math/math.h, which would define __signbitl again.
I couldn't force nldbl to build but replacing sysdeps/ieee754/ldbl-128/s_signbitl.c
with nldlb-signbit.c confirmed the #undef fixes the issue.
Wilco
On Fri, 22 May 2015, Wilco Dijkstra wrote:
> > Why the #undef? I don't see any headers with macro definitions of
> > __signbitl that need to be removed in math.h.
>
> Unless nldbl support is dead we need it as
> sysdeps/ldbl-opt/nldbl-signbit.c does:
>
> #define __signbitl __signbitl_XXX
> #include "nldbl-compat.h"
>
> nldbl-compat.h includes math/math.h, which would define __signbitl
> again. I couldn't force nldbl to build but replacing
> sysdeps/ieee754/ldbl-128/s_signbitl.c with nldlb-signbit.c confirmed the
> #undef fixes the issue.
Thanks - I was only looking in headers for a #define. I think the nldbl
code is for linking -mlong-double-64 objects with libm, although I'm not
aware of any documentation for it. Having a #undef in an installed header
that's only relevant for building glibc seems unfortunate. Maybe it would
be cleaner to (a) convert all glibc-internal calls to __signbit* into
calls to the signbit macro (which should make no difference to the code
generated at all), then (b) conditionally change the definition of the
signbit macro to use __builtin_signbit*, rather than ever defining
__signbit* as macros.
> Joseph Myers wrote:
> On Fri, 22 May 2015, Wilco Dijkstra wrote:
>
> > > Why the #undef? I don't see any headers with macro definitions of
> > > __signbitl that need to be removed in math.h.
> >
> > Unless nldbl support is dead we need it as
> > sysdeps/ldbl-opt/nldbl-signbit.c does:
> >
> > #define __signbitl __signbitl_XXX
> > #include "nldbl-compat.h"
> >
> > nldbl-compat.h includes math/math.h, which would define __signbitl
> > again. I couldn't force nldbl to build but replacing
> > sysdeps/ieee754/ldbl-128/s_signbitl.c with nldlb-signbit.c confirmed the
> > #undef fixes the issue.
>
> Thanks - I was only looking in headers for a #define. I think the nldbl
> code is for linking -mlong-double-64 objects with libm, although I'm not
> aware of any documentation for it. Having a #undef in an installed header
> that's only relevant for building glibc seems unfortunate. Maybe it would
> be cleaner to (a) convert all glibc-internal calls to __signbit* into
> calls to the signbit macro (which should make no difference to the code
> generated at all), then (b) conditionally change the definition of the
> signbit macro to use __builtin_signbit*, rather than ever defining
> __signbit* as macros.
Yes (a) seems like a good idea - I'll commit a trivial no-diff patch that does
that for __isinf*, __isnan*, __finite* and __signbit*. __copysign* and __isinf_ns
need more work as they are currently inlined inside GLIBC, so I'll leave those
for later.
The generic signbit definitions should use the built-ins like I did for fabs
(in principle the target assembler implementations could be removed as the
generic implementation using the builtin should be as good for all targets).
Also we could remove all the target math-inlines of __signbit as they are no
longer used when doing (b) given there is general agreement on no longer doing
header optimizations for older GCC versions.
Wilco
@@ -493,6 +493,13 @@ extern int matherr (struct exception *__exc);
fpclassify (__u) == FP_NAN || fpclassify (__v) == FP_NAN; }))
# endif
+# if __GNUC_PREREQ (4,0)
+# undef __signbitl
+# define __signbit(x) __builtin_signbit(x)
+# define __signbitf(x) __builtin_signbitf(x)
+# define __signbitl(x) __builtin_signbitl(x)
+# endif
+
#endif
__END_DECLS
@@ -18,9 +18,10 @@
<http://www.gnu.org/licenses/>. */
#include <math.h>
-
#include <math_private.h>
+#undef __signbit
+
int
__signbit (double x)
{
@@ -18,9 +18,10 @@
<http://www.gnu.org/licenses/>. */
#include <math.h>
-
#include <math_private.h>
+#undef __signbitf
+
int
__signbitf (float x)
{
@@ -18,9 +18,10 @@
<http://www.gnu.org/licenses/>. */
#include <math.h>
-
#include <math_private.h>
+#undef __signbitl
+
int
__signbitl (long double x)
{
@@ -21,6 +21,8 @@
#include <math_private.h>
#include <math_ldbl_opt.h>
+#undef __signbitl
+
int
___signbitl (long double x)
{
@@ -1,6 +1,7 @@
#include <math_ldbl_opt.h>
#undef weak_alias
#define weak_alias(n,a)
+#undef __signbitl
#define __signbitl(arg) ___signbitl(arg)
#include <sysdeps/ieee754/ldbl-128/s_signbitl.c>
#undef __signbitl
@@ -18,9 +18,10 @@
<http://www.gnu.org/licenses/>. */
#include <math.h>
-
#include <math_private.h>
+#undef __signbitl
+
int
__signbitl (long double x)
{