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

["Gabriel F. T. Gomes" <gftg at linux dot vnet dot ibm dot com>]

On Fri, 28 Apr 2017 22:49:13 +0000
Joseph Myers <joseph@codesourcery.com> wrote:

> On Fri, 28 Apr 2017, Gabriel F. T. Gomes wrote:
> 
> > diff --git a/bits/floatn.h b/bits/floatn.h
> > --- /dev/null
> > +++ b/bits/floatn.h
> > @@ -0,0 +1,30 @@
> > +/* Macros to control TS 18661-3 glibc features.
> > +   Copyright (C) 2016 Free Software Foundation, Inc.  
> 
> Everything in any of these patches needs to include 2017 in copyright 
> dates.

OK.

> > +/* Defined if the current compiler invocation supports _Float128 or
> > +   __float128 as an additional floating-point type.  */
> > +#define __HAVE_FLOAT128 0
> > +
> > +/* Defined if support for _Float128 is present.  This implies
> > +   support for ISO/IEC TS-18661-3, meaning that architectures that
> > +   define this macro use __GLIBC_USE (IEC_60559_TYPES_EXT).  */
> > +#define __USE_FLOAT128 0  
> 
> I still don't follow what the exact semantics of the macros are meant to 
> be.
> 
> What's "additional" for types?  Is the case of _Float128 being 
> ABI-compatible with long double included or not?  And what does "support 
> for _Float128 is present" mean?
> 
> I think the following questions might be answered by such macros, and it 
> must be completely unambiguous from the comments on the default 
> definitions exactly which question or questions each macro relates to.
> 
> (a) Does the compiler support a particular type name (such as _Float128)?
> 
> (b) Does the compiler support some name for a type that is ABI-compatible 
> with _Float128 (the name possibly being __float128 or long double)?

Actually, we've been using __HAVE_FLOAT128 as ((a) || (b)) and the
comments aren't helping.
What about this:

/* Defined if the current compiler invocation provides a 128-bit
   floating point type compatible with IEEE 754.  */
#define __HAVE_FLOAT128 0

> (c) Does this glibc build include support for _Float128 (that is, *f128 
> functions / aliases, and all the associated header pieces)?  If 
> ABI-distinct, the headers *should* care about this even when the functions 
> aren't going to be declared, because of the type-generic <math.h> macros 
> that might need to call __isinff128 etc.

We don't have a macro that indicates that support for _Float128 is
provided in our glibc builds.  This information is currently implicit.

We now understand that this is useful for glibc users.  Do you have any
suggestion for the macro name?

> (d) Is such a type, given that it is supported, ABI-distinct from the 
> default float, double and long double in the glibc ABI?  Headers care 
> about this because __isinff128 won't exist if it's not ABI-distinct.  But 
> for your purposes you could hardcode the answer that this is "yes" 
> whenever (c) is true.

Likewise, this information is implicit.  Any suggestions for the macro
name?

> (e) Should this compilation expose declarations for the *f128 interfaces?  
> I think it's best for this to be represented as ((b) && (c)) && 
> __GLIBC_USE (IEC_60559_TYPES_EXT), rather than having its own macro.
> 
> It looks rather like you're using __USE_FLOAT128 as (e), which as noted I 
> don't think should have its own macro at all,

We decided to provide this macro to avoid having to write all the
condition multiple times in the code.  We could change this, but is it
the best thing to do?

> ((b) && (d) is the condition for including 
> bits/mathcalls-helper-functions.h, (b) && (e) is the condition for 
> including bits/mathcalls.h.)

OK.

> So define macros for compiler+library support for _Float128 ((b) && (c)), 
> and compiler+library support for ABI-distinct _Float128 ((b) && (d)), both 
> of those macros admitting the possibility that the name might be different 
> - the macro for ((b) && (c)) would be used in conjunction with __GLIBC_USE 
> (IEC_60559_TYPES_EXT) to determine whether to expose public declarations, 
> the macro for ((b) && (d)) would determine whether to include 
> bits/mathcalls-helper-functions.h and how __MATH_TG is defined.  The 
> compiler+ part is relevant because you don't want to try declaring the 
> functions etc. if the compiler doesn't support the types at all.  (The 
> compiler used to build glibc must support them, possibly under another 
> name, but the user's compiler may be older.)

OK.

> > diff --git a/bits/huge_val_flt128.h b/bits/huge_val_flt128.h  
> 
> > +#ifdef __USE_FLOAT128
> > +# define HUGE_VAL_F128 (__builtin_huge_valf128 ())
> > +#endif  
> 
> I'd expect exactly one of math.h and this header to have the ((b) && (c)) 
> && __GLIBC_USE (IEC_60559_TYPES_EXT) conditional.  Probably math.h as 
> that's the existing practice for bits/huge_vall.h.  You currently have 
> conditionals in both places.

OK.

> > +#if __USE_FLOAT128 && __USE_GNU  
> 
> __USE_GNU needs to be tested with "defined", it's a defined/undefined 
> macro not 1/0.

OK.

> > diff --git a/sysdeps/powerpc/powerpc64le/bits/floatn-compat.h b/sysdeps/powerpc/powerpc64le/bits/floatn-compat.h
> > --- /dev/null
> > +++ b/sysdeps/powerpc/powerpc64le/bits/floatn-compat.h  
> 
> > +#  define L(x) x##Q  
> 
> "L" is in the user's namespace and should not go in a public header.

OK.

> > +/* Builtin __builtin_huge_val is only type-generic since GCC 7.0.  */  
> 
> It's never type-generic.  Type-generic built-in functions are only 
> type-generic on argument types (they all have int return type).

Indeed.  I am going to rewrite this comment.