This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v3 4/8] float128: Add public _Float128 declarations to libm.


I don't see patch 7 in this patch set.  When resending a patch series, 
please make the resent series self-contained, even if some patches didn't 
get changed.  (And really it would be best to say patch N/5 rather than /8 
if the first three patches are now in, so no longer part of the new 
series.)

On Tue, 9 May 2017, Gabriel F. T. Gomes wrote:

> +# ifndef __FLOATN_TYPE

The preferred convention where possible is macros defined to nonzero or 0, 
rather than defined/undefined.  So for consistency with 
__MATH_DECLARING_DOUBLE, I think you should have a macro 
__MATH_DECLARING_FLOATN, defined to 1 when bits/mathcalls.h (etc.) is 
included for _Float128, 0 when it is included for float / double / long 
double.

I think this patch is a better place than patch 8 to include the 
definition of SNANF128.

> +/* Include the file of declarations again, this time using `_Float128'
> +   instead of `double' and appending f128 to each function name.  */
> +
> +#if __HAVE_DISTINCT_FLOAT128

That's not the right logically condition for when to declare (subject to 
__GLIBC_USE (IEC_60559_TYPES_EXT)) most functions.  I think 
__HAVE_FLOAT128 && !defined _LIBC might be better, by analogy with the 
conditions for declaring long double functions.  (long double tests 
_LIBC_TEST as well, but _LIBC_TEST use ought to be obsolete after Zack's 
patch to avoid internal declarations, including _LIBC, for most of the 
testsuite).

> +# include <bits/mathcalls-helper-functions.h>

Then, you'd add a test for __HAVE_DISTINCT_FLOAT128 around this include.

> +# if __GLIBC_USE (IEC_60559_TYPES_EXT)
> +#  include <bits/mathcalls.h>
> +# endif

And this one would stay as-is.

> +#if __HAVE_FLOAT128 && __GLIBC_USE (IEC_60559_TYPES_EXT) && defined __USE_GNU
> +# if defined __GNUC__ && !__GNUC_PREREQ (7,0)
> +#  define __f128(x) x ## q
> +# else
> +#  define __f128(x) x ## f128
> +# endif

__USE_GNU implies __GLIBC_USE (IEC_60559_TYPES_EXT), so you don't need to 
test the latter here.

You have a default #undef of __CFLOAT128 in bits/floatn.h, but __f128 in 
math.h and __L in bits/floatn-compat.h.  We should have a coherent design 
for where the definitions of helper macros such as __CFLOAT128 and __f128 
go.  I wonder about the following:

* bits/floatn.h defines __f128 (when _Float128 is supported) to the 
definition concatenating f128 as a suffix.  The default bits/floatn.h 
would need a comment about this (but no definition).  It defines 
__CFLOAT128 to _Complex _Float128 when _Float128 is supported (the default 
is #undef when _Float128 is not supported) - not just for old compilers.

* bits/floatn-compat.h gives a #error if bits/floatn.h hasn't already been 
included.  It defines such things as __builtin_huge_valf128 that need 
overriding for old compilers.  In the old compiler case, it undefines and 
redefines __f128 and __CFLOAT128.  For the new compiler case, it has 
nothing to do.  __L disappears (replaced by __f128).

You also need to make headers that need bits/floatn-compat.h include it; I 
don't see any #includes of bits/floatn-compat.h in this patch.

Or maybe nothing actually requires bits/floatn.h and bits/floatn-compat.h 
to be separate and all the definitions should just go in bits/floatn.h 
unless and until there are two architectures that need different versions 
of one of the headers but the same version of the other.  That would avoid 
various places needing to #include bits/floatn-compat.h, and avoid the 
need to get includes in the correct order.

> diff --git a/sysdeps/powerpc/powerpc64le/bits/floatn-compat.h b/sysdeps/powerpc/powerpc64le/bits/floatn-compat.h

I think this file would better be in one of the patches actually adding 
support on powerpc rather than in the present series, but I may as well 
review the contents now.

> new file mode 100644
> index 0000000..7a711a8
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64le/bits/floatn-compat.h

GCC supports a range of multilib configurations with both BE and LE 
libraries for powerpc64.

In such configurations, the same headers are shared between multilibs.  So 
you can't have a separate version of an installed header for powerpc64le.  
Instead, you need to have one header with appropriate preprocessor 
conditionals that express that _Float128 is supported for powerpc64le, but 
not for BE.  And it would go in sysdeps/powerpc so all powerpc 
configurations get that same header installed.

> +#if __HAVE_FLOAT128
> +
> +/* The type _Float128, as well as the literal suffix (F128), exist for powerpc
> +   only since GCC 7.0.  */
> +# if !__GNUC_PREREQ (7, 0)
> +#  define __L(x) x##Q
> +typedef __float128 _Float128;
> +# else
> +#  define __L(x) x##F128
> +# endif

As noted, combine with __f128.

> +/* Add a typedef for older GCC compilers which don't natively support
> +   _Complex _Float128.  */
> +# if !__GNUC_PREREQ (7, 0) && !defined __CFLOAT128
> +typedef _Complex float __cfloat128 __attribute__ ((mode (KC)));

This is not namespace-clean; you need to use __mode__ (__KC__).

> +/* __builtin_signbit is type generic in GCC 6.2, which is the minimum
> +   version required for float128 support on powerpc64le.  */
> +# define __builtin_signbitf128 __builtin_signbit

I think a better approach would be to have math.h use type-generic 
__builtin_signbit (without __MATH_TG) if __GNUC_PREREQ (6,0).  Then 
nothing will ever try to call __builtin_signbitf128, because bits/floatn.h 
won't declare any _Float128 support for GCC versions without type-generic 
__builtin_signbit, and so __MATH_TG will be defined for such versions 
without _Float128 support.

-- 
Joseph S. Myers
joseph@codesourcery.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]