Skip to content
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

[WIP] Task/rhornung67/device numeric limits #1196

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

rhornung67
Copy link
Member

@rhornung67 rhornung67 commented Oct 4, 2023

Summary

NOTE: There is more work to do here. Asking for folks to comment before I plow through converting to axom::numeric_limits everywhere. Please let me know what you think.

  • This PR adds a type alias layer to be able to access C++ std::numeric_limits functionality in host and device code (CUDA and HIP) in a syntactically identical way.
  • This will enable us to replace our usage of std::numeric_limits with axom::numeric_limits for consistency across the code and to prevent unseemly compiler warnings.

Resolves #1195

Also, see #1338

@rhornung67
Copy link
Member Author

rhornung67 commented Oct 5, 2023

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.


namespace axom
{
#if defined(AXOM_USE_CUDA) && defined(AXOM_DEVICE_CODE)
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

@rhornung67 rhornung67 Oct 6, 2023

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.

Copy link
Contributor

@gunney1 gunney1 left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace C++ 'std' methods with Axom core methods in host-device functions.
3 participants