-
Notifications
You must be signed in to change notification settings - Fork 407
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
Update numerics support for __float128
#5081
Conversation
Add round, logb, nextafter, copysign, and signbit
Added denorm_min, reciprocal_overflow_threshold, quiet_NaN, and signaling_NaN
I marked it as work in progress because I did not deal with Also I might want to add specializations for the math constants. |
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.
Investigating the discrepancies with inv_pi
and sqrt2
.
The implementation I proposed has the digits from the <numbers>
header in libstdc++
I suggest we don't get hung up with this. I will investigate and fix later as needed.
STATIC_ASSERT(Kokkos::Experimental::log2e_v <__float128> == M_LOG2Eq); | ||
STATIC_ASSERT(Kokkos::Experimental::log10e_v<__float128> == M_LOG10Eq); | ||
STATIC_ASSERT(Kokkos::Experimental::pi_v <__float128> == M_PIq); | ||
// STATIC_ASSERT(Kokkos::Experimental::inv_pi_v<__float128> == M_1_PIq); // FIXME |
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 failed due to requirement 'Kokkos::Experimental::inv_pi_v<__float128> == 0.318309886183790671537767526745028689Q' ""
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.
This is ready. Failure is unrelated (no space left on device on a build that does not have these changes in the code path) |
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.
This looks fine to me, thanks
@@ -50,6 +50,8 @@ | |||
|
|||
#include <gtest/gtest.h> | |||
|
|||
namespace { |
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.
What's the point of doing this?
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.
Anonymous namespace in case some of the functions are redefined in different headers?
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.
In a unit test?
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.
You will find this idiom in the googletest samples (example here)
The definitions in that source file do not need access in any other translation unit, it avoids symbols collision (admittedly not likely to happen here). The way I see it, it falls under the general guideline of keeping scopes small.
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.
Sure but you do it in .cc (like the example you show) not in the header.
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.
Well this file is a source file in disguise. Our usual pattern of generating
// generated Backend_SomeUnitTest.cpp or such
#include <BackendSpecificHeaderWithMacroDefinitions.hpp>
#include <SomeUnitTest.hpp>
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.
It's possible for multiple unit tests to get linked together. If each object file ensures that all of the symbols it contains are private, we're guaranteed not to encounter (incredibly hard to debug, from experience) ODR violation issues. There's minimal cost, and it avoids a bad problem
Fix #5080
Note that 4b758fb is technically a defect in 3.6.00
I am not sure whether we should bother with cherry-picking in 3.6.01, I think we can live without it.