Message ID  20171103131604.144121gabriel@inconstante.eti.br 

State  New 
Headers  show 
Series 

Related  show 
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 Conly >builtin. This is true when float128 is provided as an ABIdistinct type >from long double. > >Moreover, the comparison macros from ISO C take two floatingpoint >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 compilationtime, to the floatingpoint 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 Conly builtin. Moreover, >+ the comparison macros from ISO C take two floatingpoint 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 compilationtime for >+ the correct floatingpoint 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 doubleunderscore 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 Wsystemheaders. 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 nontemplate 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 floatingpoint 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 186611 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 typegeneric macros, libstdc++, not glibc, deals with providing the overloaded function versions for C++ code. TS 186611 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 classbased, not following the C _FloatN, _FloatNx, given that C++ chose classbased 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 classbased 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 186611 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 typegeneric macros, libstdc++, not glibc, deals with >providing the overloaded function versions for C++ code. TS 186611 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 >classbased, not following the C _FloatN, _FloatNx, given that C++ chose >classbased 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 classbased 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 typegeneric macros, libstdc++, not glibc, deals with > > providing the overloaded function versions for C++ code. TS 186611 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 builtin 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 fnomatherrno. 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 builtin 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
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 Conly builtin. Moreover, + the comparison macros from ISO C take two floatingpoint 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 compilationtime for + the correct floatingpoint 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 +__iseqsig_type (float fmt, __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) + #endif __END_DECLS