-
Notifications
You must be signed in to change notification settings - Fork 27
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
Task/rhornung67/device numeric limits #1196
Conversation
With the changes in this PR, is there any reason we need this stuff: https://github.com/LLNL/axom/blob/develop/src/axom/core/numerics/floating_point_limits.hpp ? We are currently not consistent across the code. |
src/axom/core/NumericLimits.hpp
Outdated
|
||
namespace axom | ||
{ | ||
#if defined(AXOM_USE_CUDA) && defined(AXOM_DEVICE_CODE) |
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.
Would it make sense to remove if defined(AXOM_DEVICE_CODE)
and just have the AXOM_USE_CUDA
guard? It's my impression that cuda::std
should work on both the host and device.
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.
If it does, that would be preferable. I need to look into that. @kennyweiss discussed this PR yesterday. That resulted in some concerns about several things in the code that we should discuss as a team. Unfortunately, that will have to wait for a couple of weeks as next Monday is a LLNL holiday and NECDC is the week after that.
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.
@publixsubfan cuda::std
does work in device and host code. I ran into some issues with some Axom tests where cuda::std::numeric_limits
does not support long double
. The intent of my change was to use std::numeric_limits
in host code for all builds. However, it's not clear to me that we need to support long double
. long double is automatically converted to double in device code and attempting to pass long double data between host and device code is problematic.
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 @rhornung67 This looks useful. I hope the long double issue is not a blocker.
…NL/axom into task/rhornung67/device-numeric-limits
Folks, could I get another pair of eyeballs (at least) to review this PR? |
// configured with CUDA enabled. No need to rely on two different | ||
// header files in that case. | ||
template <typename T> | ||
using numeric_limits = cuda::std::numeric_limits<T>; |
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.
Is the point of using CUDA-specific numeric limits to get host device on them? Or, does that not matter since they are probably all constexpr? Does HIP have a header like this? Thanks.
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.
The main point of this is to prevent compiler warnings (calling host function from host-device function) that a few users have reported. It is not needed for HIP since the amdclang compiler is a unified host-device compiler and know how to sort everything out.
Summary
std::numeric_limits
withaxom::numeric_limits
for consistency across the code and to prevent unseemly compiler warnings.Resolves #1195
Also, see #1338