Re: [PATCH 4/4] powerpc64le: Enable float128

[Joseph Myers <joseph at codesourcery dot com>]

On Wed, 14 Jun 2017, Gabriel F. T. Gomes wrote:

> From: "Paul E. Murphy" <murphyp@linux.vnet.ibm.com>
> 
> This patch adds ULPs for the float128 type, updates the abilist for libc
> and libm, and adds the files bits/floatn.h and float128-abi.h, in order to
> enable the new type for powerpc64le.
> 
> This patch also adds the implementation of sqrtf128 for powerpc64le, since
> it is not implemented in libgcc.  The sfp-machine.h header is taken from
> libgcc.
> 
> Tested for powerpc64, powerpc64le and s390x.

Have you tested with both GCC 6 and GCC 7?

> +* Support for ISO/IEC TS 18661-3:2015 is added.  It is currently only
> +  enabled for powerpc64le.

Well, no, support is added *for the float128 interfaces corresponding to 
interfaces already implemented in glibc for other types*.  That's a subset 
of TS 18661-3 support and the summary needs to reflect that it is such a 
subset.  I'd say something like "Support is added, on powerpc64le, for 
interfaces supporting the _Float128 type from ISO/IEC TS 18661-3:2015.  
Most of the interfaces are taken from TS 18661-3.  The type-generic macros 
in <math.h> support this type, but those in <tgmath.h> do not.  The GNU C 
Library now requires GCC 6.2 or later to build for powerpc64le.  When used 
with GCC versions before GCC 7, these interfaces may be used with the type 
under the non-standard name __float128.".  Then list the interfaces, as 
you do, but remembering the HUGE_VAL interfaces (standard) and M_* 
(extension).

> diff --git a/math/test-float128.h b/math/test-float128.h
> index 17c5928..f9a7e16 100644
> --- a/math/test-float128.h
> +++ b/math/test-float128.h
> @@ -20,6 +20,8 @@
>  #include "test-math-floatn.h"
>  
>  /* Fixup builtins and constants for older compilers.  */
> +#define __GLIBC_INTERNAL_STARTING_HEADER_IMPLEMENTATION
> +#include <bits/libc-header-start.h>

This should never be needed outside of installed headers.  If you find you 
need it, that indicates something is wrong elsewhere.  <bits/floatn.h> 
should not depend on anything from <bits/libc-header-start.h>.  
__GLIBC_USE conditionals affected by <bits/libc-header-start.h> should 
only appear in installed headers that themselves include it.

> +/* Defined to 1 if the current compiler invocation provides a
> +   floating-point type with the IEEE 754 binary128 format, and this glibc
> +   includes corresponding *f128 interfaces for it.  */
> +#if defined _ARCH_PWR8 && defined __LITTLE_ENDIAN__ && (_CALL_ELF == 2) \
> +    && defined __FLOAT128__ && !defined __cplusplus
> +# define __HAVE_FLOAT128 1
> +#else
> +# define __HAVE_FLOAT128 0
> +#endif

Actually, I'd think it's desirable to be able to use the functions with 
C++ (albeit that libstdc++ support would be its own project, but one that 
depends on the functions being visible for C++).  Which means not having 
the __cplusplus conditional here, but instead using __float128 type name, 
q suffix and __attribute__ ((__mode__ (__KC__))) for C++ even with GCC 7 
and later.  (The new built-in functions should work with GCC 7 for both C 
and C++.)

> +# float128 requires adding a handful of extra flags.
> +%f128.o %f128.os %f128_r.o %f128_r.os: CFLAGS += -mfloat128

I think you need to use $(all-object-suffixes) or similar.  This looks 
like it would fail to add the option for .op objects (--enable-profile).

Or, more simply, you could just use -mfloat128 globally for building all 
of glibc for powerpc64le, just as sysdeps/ieee754/ldbl-128ibm/Makefile 
uses -mlong-double-128 unconditionally in sysdep-CFLAGS.

-- 
Joseph S. Myers
joseph@codesourcery.com