-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libc++][math] Fix undue overflowing of std::hypot(x,y,z)
#93350
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-libcxx Author: None (PaulXiCao) ChangesCloses #92782. The 3-dimentionsional The idea is to to scale the arguments (see linked issue for full discussion). Tests have been added for problematic over- and underflows. Full diff: https://github.com/llvm/llvm-project/pull/93350.diff 3 Files Affected:
diff --git a/libcxx/include/__math/hypot.h b/libcxx/include/__math/hypot.h
index 8e2c0f5b6b6d8..6b68e7bfed341 100644
--- a/libcxx/include/__math/hypot.h
+++ b/libcxx/include/__math/hypot.h
@@ -9,11 +9,16 @@
#ifndef _LIBCPP___MATH_HYPOT_H
#define _LIBCPP___MATH_HYPOT_H
+#include <__algorithm/max.h>
#include <__config>
+#include <__math/abs.h>
+#include <__math/roots.h>
#include <__type_traits/enable_if.h>
#include <__type_traits/is_arithmetic.h>
#include <__type_traits/is_same.h>
#include <__type_traits/promote.h>
+#include <array>
+#include <limits>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
@@ -41,6 +46,79 @@ inline _LIBCPP_HIDE_FROM_ABI typename __promote<_A1, _A2>::type hypot(_A1 __x, _
return __math::hypot((__result_type)__x, (__result_type)__y);
}
+#if _LIBCPP_STD_VER >= 17
+template <class _Real>
+struct __hypot_factors {
+ _Real __threshold;
+ _Real __scale_xyz;
+ _Real __scale_M;
+};
+
+// returns [underflow_factors, overflow_factors]
+template <class _Real>
+std::array<__hypot_factors<_Real>, 2> __create_factors() {
+ static_assert(std::numeric_limits<_Real>::is_iec559);
+
+ __hypot_factors<_Real> __underflow, __overflow;
+ if constexpr (std::is_same_v<_Real, float>) {
+ static_assert(-125 == std::numeric_limits<_Real>::min_exponent);
+ static_assert(+128 == std::numeric_limits<_Real>::max_exponent);
+ __underflow = __hypot_factors<_Real>{0x1.0p-62f, 0x1.0p70f, 0x1.0p-70f};
+ __overflow = __hypot_factors<_Real>{0x1.0p62f, 0x1.0p-70f, 0x1.0p+70f};
+ } else if constexpr (std::is_same_v<_Real, double>) {
+ static_assert(-1021 == std::numeric_limits<_Real>::min_exponent);
+ static_assert(+1024 == std::numeric_limits<_Real>::max_exponent);
+ __underflow = __hypot_factors<_Real>{0x1.0p-510, 0x1.0p600, 0x1.0p-600};
+ __overflow = __hypot_factors<_Real>{0x1.0p510, 0x1.0p-600, 0x1.0p+600};
+ } else { // long double
+ static_assert(std::is_same_v<_Real, long double>);
+ static_assert(-16'381 == std::numeric_limits<_Real>::min_exponent);
+ static_assert(+16'384 == std::numeric_limits<_Real>::max_exponent);
+ __underflow = __hypot_factors<_Real>{0x1.0p-8'190l, 0x1.0p9'000l, 0x1.0p-9'000l};
+ __overflow = __hypot_factors<_Real>{0x1.0p8'190l, 0x1.0p-9'000l, 0x1.0p+9'000l};
+ }
+ return {__underflow, __overflow};
+}
+
+template <class _Real>
+_Real __hypot(_Real __x, _Real __y, _Real __z) {
+ const auto [__underflow, __overflow] = __create_factors<_Real>();
+ _Real __M = std::max({__math::fabs(__x), __math::fabs(__y), __math::fabs(__z)});
+ if (__M > __overflow.__threshold) { // x*x + y*y + z*z might overflow
+ __x *= __overflow.__scale_xyz;
+ __y *= __overflow.__scale_xyz;
+ __z *= __overflow.__scale_xyz;
+ __M = __overflow.__scale_M;
+ } else if (__M < __underflow.__threshold) { // x*x + y*y + z*z might underflow
+ __x *= __underflow.__scale_xyz;
+ __y *= __underflow.__scale_xyz;
+ __z *= __underflow.__scale_xyz;
+ __M = __underflow.__scale_M;
+ } else
+ __M = 1;
+ return __M * __math::sqrt(__x * __x + __y * __y + __z * __z);
+}
+
+inline _LIBCPP_HIDE_FROM_ABI float hypot(float __x, float __y, float __z) { return __hypot(__x, __y, __z); }
+
+inline _LIBCPP_HIDE_FROM_ABI double hypot(double __x, double __y, double __z) { return __hypot(__x, __y, __z); }
+
+inline _LIBCPP_HIDE_FROM_ABI long double hypot(long double __x, long double __y, long double __z) {
+ return __hypot(__x, __y, __z);
+}
+
+template <class _A1,
+ class _A2,
+ class _A3,
+ std::enable_if_t< is_arithmetic_v<_A1> && is_arithmetic_v<_A2> && is_arithmetic_v<_A3>, int> = 0 >
+_LIBCPP_HIDE_FROM_ABI typename __promote<_A1, _A2, _A3>::type hypot(_A1 __x, _A2 __y, _A3 __z) _NOEXCEPT {
+ using __result_type = typename __promote<_A1, _A2, _A3>::type;
+ static_assert(!(
+ std::is_same_v<_A1, __result_type> && std::is_same_v<_A2, __result_type> && std::is_same_v<_A3, __result_type>));
+ return __hypot(static_cast<__result_type>(__x), static_cast<__result_type>(__y), static_cast<__result_type>(__z));
+}
+#endif
+
} // namespace __math
_LIBCPP_END_NAMESPACE_STD
diff --git a/libcxx/include/cmath b/libcxx/include/cmath
index dd194bbb55896..49f35caa7b74e 100644
--- a/libcxx/include/cmath
+++ b/libcxx/include/cmath
@@ -305,6 +305,7 @@ constexpr long double lerp(long double a, long double b, long double t) noexcept
*/
#include <__config>
+#include <__math/hypot.h>
#include <__type_traits/enable_if.h>
#include <__type_traits/is_arithmetic.h>
#include <__type_traits/is_constant_evaluated.h>
@@ -544,30 +545,6 @@ using ::scalbnl _LIBCPP_USING_IF_EXISTS;
using ::tgammal _LIBCPP_USING_IF_EXISTS;
using ::truncl _LIBCPP_USING_IF_EXISTS;
-#if _LIBCPP_STD_VER >= 17
-inline _LIBCPP_HIDE_FROM_ABI float hypot(float __x, float __y, float __z) {
- return sqrt(__x * __x + __y * __y + __z * __z);
-}
-inline _LIBCPP_HIDE_FROM_ABI double hypot(double __x, double __y, double __z) {
- return sqrt(__x * __x + __y * __y + __z * __z);
-}
-inline _LIBCPP_HIDE_FROM_ABI long double hypot(long double __x, long double __y, long double __z) {
- return sqrt(__x * __x + __y * __y + __z * __z);
-}
-
-template <class _A1, class _A2, class _A3>
-inline _LIBCPP_HIDE_FROM_ABI
- typename enable_if_t< is_arithmetic<_A1>::value && is_arithmetic<_A2>::value && is_arithmetic<_A3>::value,
- __promote<_A1, _A2, _A3> >::type
- hypot(_A1 __lcpp_x, _A2 __lcpp_y, _A3 __lcpp_z) _NOEXCEPT {
- typedef typename __promote<_A1, _A2, _A3>::type __result_type;
- static_assert((!(is_same<_A1, __result_type>::value && is_same<_A2, __result_type>::value &&
- is_same<_A3, __result_type>::value)),
- "");
- return std::hypot((__result_type)__lcpp_x, (__result_type)__lcpp_y, (__result_type)__lcpp_z);
-}
-#endif
-
template <class _A1, __enable_if_t<is_floating_point<_A1>::value, int> = 0>
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR bool __constexpr_isnan(_A1 __lcpp_x) _NOEXCEPT {
#if __has_builtin(__builtin_isnan)
diff --git a/libcxx/test/std/numerics/c.math/cmath.pass.cpp b/libcxx/test/std/numerics/c.math/cmath.pass.cpp
index 9379084499792..544c197ceb1af 100644
--- a/libcxx/test/std/numerics/c.math/cmath.pass.cpp
+++ b/libcxx/test/std/numerics/c.math/cmath.pass.cpp
@@ -12,14 +12,17 @@
// <cmath>
+#include <array>
#include <cmath>
#include <limits>
#include <type_traits>
#include <cassert>
+#include "fp_compare.h"
#include "test_macros.h"
#include "hexfloat.h"
#include "truncate_fp.h"
+#include "type_algorithms.h"
// convertible to int/float/double/etc
template <class T, int N=0>
@@ -1113,6 +1116,44 @@ void test_fmin()
assert(std::fmin(1,0) == 0);
}
+struct TestHypot3 {
+ template <class Real>
+ static void operator()() {
+ const auto check = [](Real elem, Real abs_tol) {
+ assert(std::isfinite(std::hypot(elem, Real(0), Real(0))));
+ assert(fptest_close(std::hypot(elem, Real(0), Real(0)), elem, abs_tol));
+ assert(std::isfinite(std::hypot(elem, elem, Real(0))));
+ assert(fptest_close(std::hypot(elem, elem, Real(0)), std::sqrt(Real(2)) * elem, abs_tol));
+ assert(std::isfinite(std::hypot(elem, elem, elem)));
+ assert(fptest_close(std::hypot(elem, elem, elem), std::sqrt(Real(3)) * elem, abs_tol));
+ };
+
+ { // check for overflow
+ const auto [elem, abs_tol] = []() -> std::array<Real, 2> {
+ if constexpr (std::is_same_v<Real, float>)
+ return {1e20f, 1e16f};
+ else if constexpr (std::is_same_v<Real, double>)
+ return {1e300, 1e287};
+ else // long double
+ return {1e4000l, 1e3985l};
+ }();
+ check(elem, abs_tol);
+ }
+
+ { // check for underflow
+ const auto [elem, abs_tol] = []() -> std::array<Real, 2> {
+ if constexpr (std::is_same_v<Real, float>)
+ return {1e-20f, 1e-24f};
+ else if constexpr (std::is_same_v<Real, double>)
+ return {1e-287, 1e-300};
+ else // long double
+ return {1e-3985l, 1e-4000l};
+ }();
+ check(elem, abs_tol);
+ }
+ }
+};
+
void test_hypot()
{
static_assert((std::is_same<decltype(std::hypot((float)0, (float)0)), float>::value), "");
@@ -1136,24 +1177,30 @@ void test_hypot()
assert(std::hypot(3,4) == 5);
#if TEST_STD_VER > 14
- static_assert((std::is_same<decltype(std::hypot((float)0, (float)0, (float)0)), float>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (bool)0, (float)0)), double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (unsigned short)0, (double)0)), double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (int)0, (long double)0)), long double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (double)0, (long)0)), double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (long double)0, (unsigned long)0)), long double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (int)0, (long long)0)), double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (int)0, (unsigned long long)0)), double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (double)0, (double)0)), double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (long double)0, (long double)0)), long double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (float)0, (double)0)), double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (float)0, (long double)0)), long double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((float)0, (double)0, (long double)0)), long double>::value), "");
- static_assert((std::is_same<decltype(std::hypot((int)0, (int)0, (int)0)), double>::value), "");
- static_assert((std::is_same<decltype(hypot(Ambiguous(), Ambiguous(), Ambiguous())), Ambiguous>::value), "");
+ // clang-format off
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (float)0, (float)0)), float>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (bool)0, (float)0)), double>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (unsigned short)0, (double)0)), double>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (int)0, (long double)0)), long double>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (double)0, (long)0)), double>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (long double)0, (unsigned long)0)), long double>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (int)0, (long long)0)), double>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (int)0, (unsigned long long)0)), double>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (double)0, (double)0)), double>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (long double)0, (long double)0)), long double>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (float)0, (double)0)), double>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (float)0, (long double)0)), long double>));
+ static_assert((std::is_same_v<decltype(std::hypot((float)0, (double)0, (long double)0)), long double>));
+ static_assert((std::is_same_v<decltype(std::hypot((int)0, (int)0, (int)0)), double>));
+ static_assert((std::is_same_v<decltype(hypot(Ambiguous(), Ambiguous(), Ambiguous())), Ambiguous>));
+ // clang-format on
assert(std::hypot(2,3,6) == 7);
assert(std::hypot(1,4,8) == 9);
+
+ // Check for undue over-/underflows of intermediate results.
+ // See discussion at https://github.com/llvm/llvm-project/issues/92782.
+ types::for_each(types::floating_point_types(), TestHypot3());
#endif
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! I have some comments. TBH, I have not really looked deeply into the implementation changes, I'm waiting for you to address some of my comments about explaining what's going on. But I'm sure I'll be OK with the changes once I understand them :).
static_assert((std::is_same<decltype(std::hypot((int)0, (int)0, (int)0)), double>::value), ""); | ||
static_assert((std::is_same<decltype(hypot(Ambiguous(), Ambiguous(), Ambiguous())), Ambiguous>::value), ""); | ||
// clang-format off | ||
static_assert((std::is_same_v<decltype(std::hypot((float)0, (float)0, (float)0)), float>)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static_assert((std::is_same_v<decltype(std::hypot((float)0, (float)0, (float)0)), float>)); | |
static_assert(std::is_same_v<decltype(std::hypot((float)0, (float)0, (float)0)), float>); |
While we're at it, you don't need double parens. Also applies to the surrounding lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that as well, but it is like this all throughout this rather large file. I tried to stay consistent to it...
What do you think if I create another mr which changes only this style issue all throughout the file later on? (And keeping it as is for this mr?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds totally reasonable!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically LGTM with still a bit of feedback.
cf378ed
to
268ca5b
Compare
@PaulXiCao This seems to be failing with many error messages that seem legitimate:
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
@ldionne Should be fixed now. |
@PaulXiCao Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
/cherry-pick 9628777 |
The 3-dimentionsional `std::hypot(x,y,z)` was sub-optimally implemented. This lead to possible over-/underflows in (intermediate) results which can be circumvented by this proposed change. The idea is to to scale the arguments (see linked issue for full discussion). Tests have been added for problematic over- and underflows. Closes llvm#92782 (cherry picked from commit 9628777)
/pull-request #100141 |
The 3-dimentionsional `std::hypot(x,y,z)` was sub-optimally implemented. This lead to possible over-/underflows in (intermediate) results which can be circumvented by this proposed change. The idea is to to scale the arguments (see linked issue for full discussion). Tests have been added for problematic over- and underflows. Closes llvm#92782
It looks like this commit breaks the following buildbots: sanitizer-ppc64le-linux build no. 1522 and sanitizer-ppc64be-linux build no. 1249. I will attempt to isolate and provide more information about the failure but could you take a look and provide a fix as soon as possible or revert if the fix will take too long. |
The 3-dimentionsional `std::hypot(x,y,z)` was sub-optimally implemented. This lead to possible over-/underflows in (intermediate) results which can be circumvented by this proposed change. The idea is to to scale the arguments (see linked issue for full discussion). Tests have been added for problematic over- and underflows. Closes llvm#92782 (cherry picked from commit 9628777)
And, for your reference, this only breaks when building libcxx on PowerPC (as far as we're aware). The sanitizer buildbots are okay on other architectures (x86, arm64):
|
I will probably not have time to work on this before the weekend. The fix is probably another special case guarded by preprocessor |
Summary: The 3-dimentionsional `std::hypot(x,y,z)` was sub-optimally implemented. This lead to possible over-/underflows in (intermediate) results which can be circumvented by this proposed change. The idea is to to scale the arguments (see linked issue for full discussion). Tests have been added for problematic over- and underflows. Closes #92782 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251315
…93350)" Summary: This reverts commit 9628777. More details in #93350, but this broke the PowerPC sanitizer bots. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250681
…lvm#93350)" This reverts commit 1031335.
…lvm#93350)" This reverts commit 9628777. More details in llvm#93350, but this broke the PowerPC sanitizer bots.
This is in relation to mr #93350. It was merged to main, but reverted because of failing sanitizer builds on PowerPC. The fix includes replacing the hard-coded threshold constants (e.g. `__overflow_threshold`) for different floating-point sizes by a general computation using `std::ldexp`. Thus, it should now work for all architectures. This has the drawback of not being `constexpr` anymore as `std::ldexp` is not implemented as `constexpr` (even though the standard mandates it for C++23). Closes #92782
…lvm#93350)" This reverts commit 9628777. More details in llvm#93350, but this broke the PowerPC sanitizer bots. (cherry picked from commit 1031335)
) This is in relation to mr llvm#93350. It was merged to main, but reverted because of failing sanitizer builds on PowerPC. The fix includes replacing the hard-coded threshold constants (e.g. `__overflow_threshold`) for different floating-point sizes by a general computation using `std::ldexp`. Thus, it should now work for all architectures. This has the drawback of not being `constexpr` anymore as `std::ldexp` is not implemented as `constexpr` (even though the standard mandates it for C++23). Closes llvm#92782 (cherry picked from commit 72825fd)
) This is in relation to mr llvm#93350. It was merged to main, but reverted because of failing sanitizer builds on PowerPC. The fix includes replacing the hard-coded threshold constants (e.g. `__overflow_threshold`) for different floating-point sizes by a general computation using `std::ldexp`. Thus, it should now work for all architectures. This has the drawback of not being `constexpr` anymore as `std::ldexp` is not implemented as `constexpr` (even though the standard mandates it for C++23). Closes llvm#92782
…lvm#93350)" This reverts commit 9628777. More details in llvm#93350, but this broke the PowerPC sanitizer bots. (cherry picked from commit 1031335)
) This is in relation to mr llvm#93350. It was merged to main, but reverted because of failing sanitizer builds on PowerPC. The fix includes replacing the hard-coded threshold constants (e.g. `__overflow_threshold`) for different floating-point sizes by a general computation using `std::ldexp`. Thus, it should now work for all architectures. This has the drawback of not being `constexpr` anymore as `std::ldexp` is not implemented as `constexpr` (even though the standard mandates it for C++23). Closes llvm#92782 (cherry picked from commit 72825fd)
) This is in relation to mr llvm#93350. It was merged to main, but reverted because of failing sanitizer builds on PowerPC. The fix includes replacing the hard-coded threshold constants (e.g. `__overflow_threshold`) for different floating-point sizes by a general computation using `std::ldexp`. Thus, it should now work for all architectures. This has the drawback of not being `constexpr` anymore as `std::ldexp` is not implemented as `constexpr` (even though the standard mandates it for C++23). Closes llvm#92782
Closes #92782.
The 3-dimentionsional
std::hypot(x,y,z)
was sub-optimally implemented. This lead to possible over-/underflows in (intermediate) results which can be circumvented by this proposed change.The idea is to to scale the arguments (see linked issue for full discussion).
Tests have been added for problematic over- and underflows.