From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id 826C73858D39; Fri, 1 Mar 2024 12:27:50 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 826C73858D39 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1709296070; bh=UDjxsZ4g7GcNRquPOQWFvtcS4XDUhpeAKcxNpluJMVc=; h=From:To:Subject:Date:In-Reply-To:References:From; b=ZsDatSZVC2geRzV88UL8YKnuEisveybylqGZAoC8zh3GrMOjCXNV03P/F+2I13l+6 vxySegO6CSBpIOZYIENCAOXz2Z3e3KJ2T0pVcX2EBeC10jJ5aCtuBMCH5z/4geeeTa rgptQ0fXdzoxJuUhqbmqoV58ktvnOHEMyo1ruZgk= From: "redi at gcc dot gnu.org" To: gcc-bugs@gcc.gnu.org Subject: [Bug libstdc++/77776] C++17 std::hypot implementation is poor Date: Fri, 01 Mar 2024 12:27:45 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gcc X-Bugzilla-Component: libstdc++ X-Bugzilla-Version: 7.0 X-Bugzilla-Keywords: X-Bugzilla-Severity: normal X-Bugzilla-Who: redi at gcc dot gnu.org X-Bugzilla-Status: ASSIGNED X-Bugzilla-Resolution: X-Bugzilla-Priority: P3 X-Bugzilla-Assigned-To: emsr at gcc dot gnu.org X-Bugzilla-Target-Milestone: --- X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 List-Id: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D77776 --- Comment #12 from Jonathan Wakely --- (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) >=20 > namespace __detail > { > template > inline _GLIBCXX_CONSTEXPR typename enable_if::val= ue, > 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); > } >=20 > template > inline _GLIBCXX_CONSTEXPR typename enable_if::val= ue, > _Tp>::type __hypot3(_Tp __x, _Tp __y, _Tp __z) noexcept > { > __x =3D std::fabs(__x); > __y =3D std::fabs(__y); > __z =3D std::fabs(__z); >=20 > const _Tp > __max =3D std::fmax(std::fmax(__x, __y), __z); >=20 > if (GCC_UNLIKELY(__max =3D=3D _Tp{})) > { > return __max; > } > else > { > __x /=3D __max; > __y /=3D __max; > __z /=3D __max; > return std::sqrt(__x*__x + __y*__y + __z*__z) * __max; > } > } > } // __detail >=20 >=20 > template > inline _GLIBCXX_CONSTEXPR typename enable_if::val= ue, > _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>, but see below. > { > return (GCC_UNLIKELY(__detail::__isinf3(__x, __y, __z))) ? > numeric_limits<_Tp>::infinity() : __detail::__hypot3(__x, __y, __z); > } >=20 > #undef GCC_UNLIKELY > #undef GCC_LIKELY >=20 > 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 m= ath > 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 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 =3D std::fabs(__x); __y =3D std::fabs(__y); __z =3D std::fabs(__z); const _Tp __max =3D std::fmax(std::fmax(__x, __y), __z); if (__max =3D=3D _Tp{}) [[__unlikely__]] return __max; else { __x /=3D __max; __y /=3D __max; __z /=3D __max; return std::sqrt(__x*__x + __y*__y + __z*__z) * __max; } } This would add a dependency on to , which isn't currently there. Maybe we could just return (_Tp)__builtin_huge_vall().=