Provide a C++ version of iseqsig

  I would like to receive some feedback on the correctness of this
implementation, more specifically on the correctness of any implicit
type conversions, which I might have missed.  I'm working on a test case
in the meantime (I only did some standalone tests outside of glibc test
suite).

-- 8< --
In C++ mode, __MATH_TG cannot be used for defining iseqsig, because
__MATH_TG relies on __builtin_types_compatible_p, which is a C-only
builtin.  This is true when float128 is provided as an ABI-distinct type
from long double.

Moreover, the comparison macros from ISO C take two floating-point
arguments, which need not have the same type.  Choosing what underlying
function to call requires evaluating the formats of the arguments, then
selecting which is wider.  The macro __MATH_EVAL_FMT2 provides this
information, however, only the type of the macro expansion is relevant
(actually evaluating the expression would be incorrect).

This patch provides a C++ version of iseqsig, in which only the type
resulted from a call to __MATH_EVAL_FMT2 is used as an additional
argument, fmt, to the helper function, __iseqsig_type.  This function
is overloaded, in compilation-time, to the floating-point type specified
by the fmt argument, then calls the appropriate underlying function (the
type of the arguments is left unchanged with the help of templates).

Tested for powerpc64le and x86_64.

	[BZ #22377]
	* math/math.h [C++] (iseqsig): New implementation, which does
	not rely on __MATH_TG/__builtin_types_compatible_p.
---
 math/math.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)
  

On 03/11/17 11:16 -0200, Gabriel F. T. Gomes wrote:
>I would like to receive some feedback on the correctness of this
>implementation, more specifically on the correctness of any implicit
>type conversions, which I might have missed.  I'm working on a test case
>in the meantime (I only did some standalone tests outside of glibc test
>suite).
>
>-- 8< --
>In C++ mode, __MATH_TG cannot be used for defining iseqsig, because
>__MATH_TG relies on __builtin_types_compatible_p, which is a C-only
>builtin.  This is true when float128 is provided as an ABI-distinct type
>from long double.
>
>Moreover, the comparison macros from ISO C take two floating-point
>arguments, which need not have the same type.  Choosing what underlying
>function to call requires evaluating the formats of the arguments, then
>selecting which is wider.  The macro __MATH_EVAL_FMT2 provides this
>information, however, only the type of the macro expansion is relevant
>(actually evaluating the expression would be incorrect).
>
>This patch provides a C++ version of iseqsig, in which only the type
>resulted from a call to __MATH_EVAL_FMT2 is used as an additional
>argument, fmt, to the helper function, __iseqsig_type.  This function
>is overloaded, in compilation-time, to the floating-point type specified
>by the fmt argument, then calls the appropriate underlying function (the
>type of the arguments is left unchanged with the help of templates).
>
>Tested for powerpc64le and x86_64.
>
>	[BZ #22377]
>	* math/math.h [C++] (iseqsig): New implementation, which does
>	not rely on __MATH_TG/__builtin_types_compatible_p.
>---
> math/math.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 49 insertions(+), 1 deletion(-)
>
>diff --git a/math/math.h b/math/math.h
>index 326fd8ebe1..4744f9a4c9 100644
>--- a/math/math.h
>+++ b/math/math.h
>@@ -1152,8 +1152,56 @@ iszero (__T __val)
>
> /* Return X == Y but raising "invalid" and setting errno if X or Y is
>    a NaN.  */
>+# ifndef __cplusplus
>+#  define iseqsig(x, y) \
>+   __MATH_TG (__MATH_EVAL_FMT2 (x, y), __iseqsig, ((x), (y)))
>+# else
>+/* In C++ mode, __MATH_TG cannot be used, because it relies on
>+   __builtin_types_compatible_p, which is a C-only builtin.  Moreover,
>+   the comparison macros from ISO C take two floating-point arguments,
>+   which need not have the same type.  Choosing what underlying function
>+   to call requires evaluating the formats of the arguments, then
>+   selecting which is wider.  The macro __MATH_EVAL_FMT2 provides this
>+   information, however, only the type of the macro expansion is
>+   relevant (actually evaluating the expression would be incorrect).
>+   Thus, the type is used in an additional argument, fmt, to the helper
>+   function, __iseqsig_type, which is overloaded in compilation-time for
>+   the correct floating-point type, then calls the appropriate
>+   underlying function (the type of the arguments is unchanged with the
>+   help of templates).  */
>+extern "C++" {
>+template <typename __T1, typename __T2> inline int

I don't know what the glibc convention is, but in libstdc++ we'd just
use _T1 here, not __T1. The double-underscore is not needed when the
first letter is uppercase.

>+__iseqsig_type (float fmt, __T1 x, __T2 y)

All the "fmt" and "x" and "y" parameters need to use reserved names,
i.e. __fmt, __x and __y.

But I wouldn't bother naming the __fmt parameter at all. It's unused,
so will produce warnings with -Wsystem-headers. C++ allows unused
parameters to be unnamed, so just say:

__iseqsig_type (float, __T1 __x, __T2 __y)

>+{
>+  return __iseqsigf (x, y);
>+}
>+template <typename __T1, typename __T2> inline int
>+__iseqsig_type (double fmt, __T1 x, __T2 y)
>+{
>+  return __iseqsig (x, y);
>+}
>+template <typename __T1, typename __T2> inline int
>+__iseqsig_type (long double fmt, __T1 x, __T2 y)
>+{
>+#  ifdef __NO_LONG_DOUBLE_MATH
>+  return __iseqsig (x, y);
>+#  else
>+  return __iseqsigl (x, y);
>+#  endif
>+}
>+#  if __HAVE_DISTINCT_FLOAT128
>+template <typename __T1, typename __T2> inline int
>+__iseqsig_type (_Float128 fmt, __T1 x, __T2 y)
>+{
>+  return __iseqsigf128 (x, y);
>+}
>+#  endif
>+}
>+# endif
>+
> # define iseqsig(x, y) \
>-  __MATH_TG (__MATH_EVAL_FMT2 (x, y), __iseqsig, ((x), (y)))
>+  __iseqsig_type (__MATH_EVAL_FMT2 (x, y), x, y)

Why define this as a macro, not an inline function template?

template<typename _T1, typename _T2>
  inline int
  iseqsig(_T1 __x, _T2 __y)
  {
    return __iseqsig_type (__MATH_EVAL_FMT2 (__x, __y), __x, __y);
  }

The C++ standard explicitly requires all functions from the C library
to be defined as real functions, and *not* macros. This means the C++
library has to do #undef for every function from the C library that
might be defined as a macro. Doing that here would make iseqsig
unusable, so a C++ implementation that wanted to define it would need
to provide its own definition. If you define an inline function it
just works, and there's no problem. Most C++ programmers want fewer
macros, not more.

Also, should it be declared to not throw exceptions?

Apart from those points, your solution will work, but it results in
two different instantiations of the function template for calls to
iseqsig(1.0, 1.0f) and iseqsig(1.0f, 1.0).

The first one will call:

  __iseqsig_type<double, float>(double, double, float);

and the second will call:

  __iseqsig_type<float, double>(double, float, double);

If they are inlined then it won't matter, but if not you'll generate
twice as much code as needed (and also more debug info).

If you're allowed to use GCC's __typeof__ (or __decltype__) then I'd
do it like this:

extern "C++" {
template<typename> struct __iseqsig_type;

template<> struct __iseqsig_type<float> {
  static int __call(float __x, float __y) throw()
  { return __iseqsigf (__x, __y); }
};

template<> struct __iseqsig_type<double> {
  static int __call(double __x, double __y) throw()
  { return __iseqsig (__x, __y); }
};

// ... long double and float128 specializations ...

template<typename _T1, typename _T2>
  inline int
  iseqsig(_T1 __x, _T2 __y) throw()
  {
    typedef __typeof__ (__MATH_EVAL_FMT2 (__x, __y)) _T3;
    return __iseqsig_type<_T3>::__call(__x, __y);
  }
}

If you can't use __typeof__, you could do what libstdc++ does for
<cmath> overloads: provide non-template functions for the cases where
the parameters are the same:

extern "C++" {
inline int iseqsig(float __x, float __y) throw() { return __iseqsigf(__x, __y); }
inline int iseqsig(double __x, double __y) throw() { return __iseqsig(__x, __y); }
// ... long double and float128 overloads ...

And then provide a function template to handle mixed types, which
converts both arguments to the promoted type:

// Handle mixed argument types.
template<typename _T1, typename _T2>
  inline int
  iseqsig(_T1 __x, _T2 __y) throw()
  {
    return iseqsig(true ? __x : __MATH_EVAL_FMT2 (__x, __y),
                   true ? __y : __MATH_EVAL_FMT2 (__x, __y));
  }
}

The conditional expression will always return the first operand, but
will promote it to the type of the second if needed.
  

On Fri, 3 Nov 2017, Gabriel F. T. Gomes wrote:

> +__iseqsig_type (float fmt, __T1 x, __T2 y)

As noted, namespace requirements mean it should be __fmt, __x, __y.

> +  __iseqsig_type (__MATH_EVAL_FMT2 (x, y), x, y)

As far as I can tell, __MATH_EVAL_FMT2 (x, y) *does* get evaluated (with 
consequent floating-point exceptions), just the value of that function 
argument isn't used.  I'd think something like

(__typeof (__MATH_EVAL_FMT2 (x, y)) 0)

(adjust to use C++ features as appropriate) would be better to ensure no 
evaluation.
  

On Fri, 3 Nov 2017, Jonathan Wakely wrote:

> The C++ standard explicitly requires all functions from the C library
> to be defined as real functions, and *not* macros. This means the C++
> library has to do #undef for every function from the C library that
> might be defined as a macro. Doing that here would make iseqsig
> unusable, so a C++ implementation that wanted to define it would need
> to provide its own definition. If you define an inline function it
> just works, and there's no problem. Most C++ programmers want fewer
> macros, not more.

With all these C++ inline functions in math.h for TS 18661-1 macros 
(issignaling, iszero, iseqsig; iscanonical only for configurations where 
it's nontrivial; not issubnormal, because that's defined in terms of 
fpclassify so has no problems with __MATH_TG given libstdc++ defines 
fpclassify as a function) there's a question of what the interactions with 
libstdc++ should look like in future.

For the C99 type-generic macros, libstdc++, not glibc, deals with 
providing the overloaded function versions for C++ code.  TS 18661-1 is 
expected to be integrated into the C standard for C2x, which I suppose 
makes it likely to become part of standard C++ as well when the C2x 
standard library is integrated into C++.  So should libstdc++ be 
responsible for providing function versions of all these macros?  Or 
should glibc do it?  (If glibc should do it, that implies it should 
provide a function version of iscanonical for C++ in the case where 
iscanonical is trivial and just converts its argument to its semantic type 
and returns 0, not just in the nontrivial cases, and that it should 
provide a function version of issubnormal for C++ although the macro 
version should actually work for C++ at present even in the presence of 
__float128 support.)

I have been making the assumption, in both the GCC and the glibc context, 
that any C++ bindings for IEEE 754 interchange and extended types will be 
class-based, not following the C _FloatN, _FloatNx, given that C++ chose 
class-based std::decimal bindings for decimal floating point (and for 
complex numbers), so that while __float128 should be supported for C++ 
code in the case where it exists with a distinct format from long double 
(and if libstdc++ supports __float128, or a new class-based binding for 
that format, it should be able to call the glibc *f128 functions as part 
of implementing that support), the C++ case in glibc does not need to deal 
with e.g. _Float64x, _Float128, long double all existing as distinct types 
with the same format (whereas the C code does need to deal with that).

> And then provide a function template to handle mixed types, which
> converts both arguments to the promoted type:

Does this achieve the desired effect of calling the function for the 
__MATH_EVAL_FMT2 type even when both arguments have the same type (but the 
__MATH_EVAL_FMT2 type is wider than that type)?
  

On 03/11/17 16:31 +0000, Joseph Myers wrote:
>On Fri, 3 Nov 2017, Jonathan Wakely wrote:
>
>> The C++ standard explicitly requires all functions from the C library
>> to be defined as real functions, and *not* macros. This means the C++
>> library has to do #undef for every function from the C library that
>> might be defined as a macro. Doing that here would make iseqsig
>> unusable, so a C++ implementation that wanted to define it would need
>> to provide its own definition. If you define an inline function it
>> just works, and there's no problem. Most C++ programmers want fewer
>> macros, not more.
>
>With all these C++ inline functions in math.h for TS 18661-1 macros
>(issignaling, iszero, iseqsig; iscanonical only for configurations where
>it's nontrivial; not issubnormal, because that's defined in terms of
>fpclassify so has no problems with __MATH_TG given libstdc++ defines
>fpclassify as a function) there's a question of what the interactions with
>libstdc++ should look like in future.
>
>For the C99 type-generic macros, libstdc++, not glibc, deals with
>providing the overloaded function versions for C++ code.  TS 18661-1 is
>expected to be integrated into the C standard for C2x, which I suppose
>makes it likely to become part of standard C++ as well when the C2x
>standard library is integrated into C++.  So should libstdc++ be
>responsible for providing function versions of all these macros?  Or

I'd be happy to define them in libstdc++, as long as we have a
__builtin_sigseq in GCC that can be used for it.

So in that case, only defining it as a macro in glibc is the right
option, as libstdc++ can just #undef it. But then we should ask if
it's worth even defining it as a macro for C++.

>should glibc do it?  (If glibc should do it, that implies it should
>provide a function version of iscanonical for C++ in the case where
>iscanonical is trivial and just converts its argument to its semantic type
>and returns 0, not just in the nontrivial cases, and that it should
>provide a function version of issubnormal for C++ although the macro
>version should actually work for C++ at present even in the presence of
>__float128 support.)
>
>I have been making the assumption, in both the GCC and the glibc context,
>that any C++ bindings for IEEE 754 interchange and extended types will be
>class-based, not following the C _FloatN, _FloatNx, given that C++ chose
>class-based std::decimal bindings for decimal floating point (and for
>complex numbers), so that while __float128 should be supported for C++
>code in the case where it exists with a distinct format from long double
>(and if libstdc++ supports __float128, or a new class-based binding for
>that format, it should be able to call the glibc *f128 functions as part
>of implementing that support), the C++ case in glibc does not need to deal
>with e.g. _Float64x, _Float128, long double all existing as distinct types
>with the same format (whereas the C code does need to deal with that).
>
>> And then provide a function template to handle mixed types, which
>> converts both arguments to the promoted type:
>
>Does this achieve the desired effect of calling the function for the
>__MATH_EVAL_FMT2 type even when both arguments have the same type (but the
>__MATH_EVAL_FMT2 type is wider than that type)?

Ah, no, it doesn't. If __typeof__(1.0f + 1.0f) is double, then it
would incorrectly call __iseqsig(float, float).

Maybe it's better to leave the C++ definitions to libstdc++, where we
can use decltype and use arbitrarily complicated template
metaprogramming that probably isn't appropriate for glibc.
  

On Fri, 3 Nov 2017, Jonathan Wakely wrote:

> > For the C99 type-generic macros, libstdc++, not glibc, deals with
> > providing the overloaded function versions for C++ code.  TS 18661-1 is
> > expected to be integrated into the C standard for C2x, which I suppose
> > makes it likely to become part of standard C++ as well when the C2x
> > standard library is integrated into C++.  So should libstdc++ be
> > responsible for providing function versions of all these macros?  Or
> 
> I'd be happy to define them in libstdc++, as long as we have a
> __builtin_sigseq in GCC that can be used for it.

There are no built-in functions in GCC for any of these operations.  For 
iseqsig, there would be the difficulty that it's defined to set errno 
(under the same circumstances where libm functions set errno); I'd imagine 
GCC should have a __builtin_iseqsig that never sets errno, and that could 
be implemented with a single comparison instruction on many processors, 
but it would only be appropriate to use to implement the macro/function 
with -fno-math-errno.  For iscanonical, when it's nontrivial I'm not 
convinced it's simple enough for inline expansion to be appropriate.

(Tamar Christina's GCC patch for built-in classification functions using 
integer arithmetic also added __builtin_iszero and __builtin_issubnormal, 
but it had to be reverted because it caused regressions on some platforms.  
See GCC bugs 77925, 77926, 77928 requesting __builtin_issubnormal, 
__builtin_iszero, __builtin_iseqsig; I don't think there's a bug 
requesting __builtin_issignaling, and as above I doubt 
__builtin_iscanonical makes sense.)
  

On 11/03/2017 10:58 PM, Jonathan Wakely wrote:
> Maybe it's better to leave the C++ definitions to libstdc++, where we
> can use decltype and use arbitrarily complicated template
> metaprogramming that probably isn't appropriate for glibc.

I think we want to fix the 2.26 release, so we need to do something 
without GCC's help here.  The bug reports suggest to me that people 
really want to use this functionality from C++.

I suppose we could restrict the definition to GNU compilers (which ave 
__typeof) or compilers which support C++11 (and thus decltype).

Thanks,
Florian