[AArch64] Add inlines for signbit (v2)

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

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