This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
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