[PATCH][AArch64] Add inlines for signbit

[PATCH][AArch64] Add inlines for signbit

[Wilco Dijkstra]

Add AArch64 inlines for __signbit, __signbitf and __signbitl which default to the GCC builtin
functions.

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).

OK for commit?

2015-04-15  Wilco Dijkstra  <wdijkstr@arm.com>

	* sysdeps/aarch64/fpu/bits/mathinline.h: New file.
	(__signbit): New function, use __builtin_signbit.
	(__signbitf): Likewise.  (__signbitl): Likewise.

---
 sysdeps/aarch64/fpu/bits/mathinline.h | 49 +++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 sysdeps/aarch64/fpu/bits/mathinline.h

diff --git a/sysdeps/aarch64/fpu/bits/mathinline.h b/sysdeps/aarch64/fpu/bits/mathinline.h
new file mode 100644
index 0000000..e3d6a3d
--- /dev/null
+++ b/sysdeps/aarch64/fpu/bits/mathinline.h
@@ -0,0 +1,49 @@
+/* Inline math functions for AArch64.
+   Copyright (C) 2015 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
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _MATH_H
+# error "Never use <bits/mathinline.h> directly; include <math.h> instead."
+#endif
+
+#ifndef __extern_inline
+# define __MATH_INLINE __inline
+#else
+# define __MATH_INLINE __extern_inline
+#endif
+
+#ifdef __USE_ISOC99
+
+__MATH_INLINE int
+__NTH (__signbitf (float __x))
+{
+  return __builtin_signbitf (__x);
+}
+
+__MATH_INLINE int
+__NTH (__signbit (double __x))
+{
+  return __builtin_signbit (__x);
+}
+
+__MATH_INLINE int
+__NTH (__signbitl (long double __x))
+{
+  return __builtin_signbitl (__x);
+}
+
+#endif /* C99 */
-- 
1.9.1

Re: [PATCH][AArch64] Add inlines for signbit

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: [PATCH][AArch64] Add inlines for signbit

[Wilco Dijkstra]

> 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).

Eg. in math.h:

#if defined __USE_ISOC99 && __GNUC_PREREQ(2,97)
# undef signbit
# ifdef __NO_LONG_DOUBLE_MATH
#  define signbit(x) \
     (sizeof (x) == sizeof (float) ? __builtin_signbitf (x) : __builtin_signbit (x))
# else
...

Wilco

RE: [PATCH][AArch64] Add inlines for signbit

[Joseph Myers]

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).

-- 
Joseph S. Myers
joseph@codesourcery.com

RE: [PATCH][AArch64] Add inlines for signbit (v2)

[Wilco Dijkstra]

> 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(-)

diff --git a/math/math.h b/math/math.h
index 7e959fc..1262f49 100644
--- a/math/math.h
+++ b/math/math.h
@@ -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
diff --git a/sysdeps/ieee754/dbl-64/s_signbit.c b/sysdeps/ieee754/dbl-64/s_signbit.c
index 764f11a..e78e998 100644
--- a/sysdeps/ieee754/dbl-64/s_signbit.c
+++ b/sysdeps/ieee754/dbl-64/s_signbit.c
@@ -18,9 +18,10 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <math.h>
-
 #include <math_private.h>
 
+#undef __signbit
+
 int
 __signbit (double x)
 {
diff --git a/sysdeps/ieee754/flt-32/s_signbitf.c b/sysdeps/ieee754/flt-32/s_signbitf.c
index 169820e..dccf0c7 100644
--- a/sysdeps/ieee754/flt-32/s_signbitf.c
+++ b/sysdeps/ieee754/flt-32/s_signbitf.c
@@ -18,9 +18,10 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <math.h>
-
 #include <math_private.h>
 
+#undef __signbitf
+
 int
 __signbitf (float x)
 {
diff --git a/sysdeps/ieee754/ldbl-128/s_signbitl.c b/sysdeps/ieee754/ldbl-128/s_signbitl.c
index acfe859..25c0bc7 100644
--- a/sysdeps/ieee754/ldbl-128/s_signbitl.c
+++ b/sysdeps/ieee754/ldbl-128/s_signbitl.c
@@ -18,9 +18,10 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <math.h>
-
 #include <math_private.h>
 
+#undef __signbitl
+
 int
 __signbitl (long double x)
 {
diff --git a/sysdeps/ieee754/ldbl-128ibm/s_signbitl.c b/sysdeps/ieee754/ldbl-128ibm/s_signbitl.c
index e95ad55..39102d2 100644
--- a/sysdeps/ieee754/ldbl-128ibm/s_signbitl.c
+++ b/sysdeps/ieee754/ldbl-128ibm/s_signbitl.c
@@ -21,6 +21,8 @@
 #include <math_private.h>
 #include <math_ldbl_opt.h>
 
+#undef __signbitl
+
 int
 ___signbitl (long double x)
 {
diff --git a/sysdeps/ieee754/ldbl-64-128/s_signbitl.c b/sysdeps/ieee754/ldbl-64-128/s_signbitl.c
index 850db73..9955ecf 100644
--- a/sysdeps/ieee754/ldbl-64-128/s_signbitl.c
+++ b/sysdeps/ieee754/ldbl-64-128/s_signbitl.c
@@ -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
diff --git a/sysdeps/ieee754/ldbl-96/s_signbitl.c b/sysdeps/ieee754/ldbl-96/s_signbitl.c
index bbe72a6..5e7a0d7 100644
--- a/sysdeps/ieee754/ldbl-96/s_signbitl.c
+++ b/sysdeps/ieee754/ldbl-96/s_signbitl.c
@@ -18,9 +18,10 @@
    <http://www.gnu.org/licenses/>.  */
 
 #include <math.h>
-
 #include <math_private.h>
 
+#undef __signbitl
+
 int
 __signbitl (long double x)
 {
-- 
1.9.1

RE: [PATCH][AArch64] Add inlines for signbit (v2)

[Joseph Myers]

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 S. Myers
joseph@codesourcery.com

RE: [PATCH][AArch64] Add inlines for signbit (v2)

[Wilco Dijkstra]

> 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

RE: [PATCH][AArch64] Add inlines for signbit (v2)

[Joseph Myers]

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 S. Myers
joseph@codesourcery.com

RE: [PATCH][AArch64] Add inlines for signbit (v2)

[Wilco Dijkstra]

> 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