[Bug libstdc++/77776] C++17 std::hypot implementation is poor

["redi at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77776

--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to g.peterhoff from comment #11)
> Would this be a good implementation for hypot3 in cmath?

Thanks for the suggestion!

> #define GCC_UNLIKELY(x) __builtin_expect(x, 0)
> #define GCC_LIKELY(x) __builtin_expect(x, 1)
> 
> namespace __detail
> {
> 	template <typename _Tp>
> 	inline _GLIBCXX_CONSTEXPR typename enable_if<is_floating_point<_Tp>::value,
> bool>::type	__isinf3(const _Tp __x, const _Tp __y, const _Tp __z)	noexcept
> 	{
> 		return bool(int(std::isinf(__x)) | int(std::isinf(__y)) |
> int(std::isinf(__z)));

The casts are redundant and just make it harder to read IMHO:

  return std::isinf(__x) | std::isinf(__y) | std::isinf(__z);



> 	}
> 
> 	template <typename _Tp>
> 	inline _GLIBCXX_CONSTEXPR typename enable_if<is_floating_point<_Tp>::value,
> _Tp>::type	__hypot3(_Tp __x, _Tp __y, _Tp __z)	noexcept
> 	{
> 		__x = std::fabs(__x);
> 		__y = std::fabs(__y);
> 		__z = std::fabs(__z);
> 
> 		const _Tp
> 			__max = std::fmax(std::fmax(__x, __y), __z);
> 
> 		if (GCC_UNLIKELY(__max == _Tp{}))
> 		{
> 			return __max;
> 		}
> 		else
> 		{
> 			__x /= __max;
> 			__y /= __max;
> 			__z /= __max;
> 			return std::sqrt(__x*__x + __y*__y + __z*__z) * __max;
> 		}
> 	}
> }	//	__detail
> 
> 
> 	template <typename _Tp>
> 	inline _GLIBCXX_CONSTEXPR typename enable_if<is_floating_point<_Tp>::value,
> _Tp>::type	__hypot3(const _Tp __x, const _Tp __y, const _Tp __z)	noexcept

This is a C++17 function, you can use enable_if_t<is_floating_point_v<_Tp>>,
but see below.

> 	{
> 		return (GCC_UNLIKELY(__detail::__isinf3(__x, __y, __z))) ?
> numeric_limits<_Tp>::infinity() : __detail::__hypot3(__x, __y, __z);
> 	}
> 
> #undef GCC_UNLIKELY
> #undef GCC_LIKELY
> 
> How does it work?
> * Basically, I first pull out the special case INFINITY (see
> https://en.cppreference.com/w/cpp/numeric/math/hypot).
> * As an additional safety measure (to prevent misuse) the functions are
> defined by enable_if.

I don't think we want to slow down compilation like that. If users decide to
misuse std::__detail::__isinf3 then they get what they deserve.


> constexpr
> * The hypot3 functions can thus be defined as _GLIBCXX_CONSTEXPR.

Just use 'constexpr' because this function isn't compiled as C++98. Then you
don't need the 'inline'. Although the standard doesn't allow std::hypot3 to be
constexpr.

> Questions
> * To get a better runtime behavior I define GCC_(UN)LIKELY. Are there
> already such macros (which I have overlooked)?

No, but you can do:


  if (__isinf3(__x, __y, __x)) [[__unlikely__]]
    ...


> * The functions are noexcept. Does that make sense? If yes: why are the math
> functions not noexcept?

I think it's just because nobody bothered to add them, and I doubt much code
ever needs to check whether they are noexcept. The compiler should already know
the standard libm functions don't throw. For this function (which isn't in libm
and the compiler doesn't know about) it seems worth adding 'noexcept'.


Does splitting it into three functions matter? It seems simpler as a single
function:

  template<typename _Tp>
    constexpr _Tp
    __hypot3(_Tp __x, _Tp __y, _Tp __z) noexcept
    {
      if (std::isinf(__x) | std::isinf(__y) | std::isinf(__z)) [[__unlikely__]]
        return numeric_limits<_Tp>::infinity();
      __x = std::fabs(__x);
      __y = std::fabs(__y);
      __z = std::fabs(__z);
      const _Tp __max = std::fmax(std::fmax(__x, __y), __z);
      if (__max == _Tp{}) [[__unlikely__]]
        return __max;
      else
        {
          __x /= __max;
          __y /= __max;
          __z /= __max;
          return std::sqrt(__x*__x + __y*__y + __z*__z) * __max;
        }
    }

This would add a dependency on <limits> to <cmath>, which isn't currently
there. Maybe we could just return (_Tp)__builtin_huge_vall().