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

Apply [[noreturn]] to Kokkos::abort when applicable #3106

Merged
merged 5 commits into from
Jul 1, 2020

Conversation

AndrewGaspar
Copy link
Contributor

I haven't run any tests beyond just building. Pushing to run CI.

I removed the __APPLE__ check for cuda_abort since I don't think macOS supports CUDA in recent versions anyway. I can put it back if this configuration is still supported, though.

@dalg24-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@dalg24
Copy link
Member

dalg24 commented Jun 17, 2020

OK to test

@masterleinad
Copy link
Contributor

I removed the __APPLE__ check for cuda_abort since I don't think macOS supports CUDA in recent versions anyway. I can put it back if this configuration is still supported, though.

I would say it does at least for CUDA versions we support: https://www.nvidia.com/en-us/drivers/cuda/418_163/macosx-cuda-418_163-driver/ and assert() is not supported on MacOS at least in 9.0 https://docs.nvidia.com/cuda/archive/9.0/cuda-c-programming-guide/index.html#assertion.

@AndrewGaspar
Copy link
Contributor Author

OK, if people are still using CUDA on Mac, then I'll put it back.

Copy link

@dhollman dhollman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use the KOKKOS_IMPL_ prefix in macros where we're not going to guarantee stability for users, which is probably the case here, at least at first (we can always add #define KOKKOS_ABORT_NORETURN KOKKOS_IMPL_ABORT_NORETURN later if we change our minds)

Otherwise, thanks! Looks good.

core/src/impl/Kokkos_Error.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_Error.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_Error.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_Error.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_Error.hpp Outdated Show resolved Hide resolved
Co-authored-by: D. S. Hollman <dshollm@sandia.gov>
core/src/Cuda/Kokkos_Cuda_abort.hpp Show resolved Hide resolved
core/src/Cuda/Kokkos_Cuda_abort.hpp Outdated Show resolved Hide resolved
@masterleinad
Copy link
Contributor

Retest this please.

@crtrott
Copy link
Member

crtrott commented Jun 30, 2020

Retest This Please

#endif

#elif defined(KOKKOS_ENABLE_HIP) && defined(__HIP_DEVICE_COMPILE__)
// HIP does not abort
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changed recently, see

__device__ __attribute__((noinline)) void hip_abort(char const *msg) {
#ifndef NDEBUG
// disable printf on release builds, as it has a non-trivial performance
// impact
printf("Aborting with message `%s'.\n", msg);
#endif
abort();
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh man. I was about to merge it :-(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine with addressing that in a follow-up pull request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am currently seeing

/tmp/kokkos/core/src/HIP/Kokkos_HIP_Abort.hpp:64:1: warning: function declared 'noreturn' should not return [-Winvalid-noreturn]

if trying to mark the function with [[noreturn]] so we might just postpone fixing it.

@crtrott crtrott added the Enhancement Improve existing capability; will potentially require voting label Jun 30, 2020
@crtrott crtrott added this to the Tentative 3.2 Release milestone Jun 30, 2020
Copy link

@dhollman dhollman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dalg24 dalg24 merged commit 48a50c9 into kokkos:develop Jul 1, 2020
@AndrewGaspar AndrewGaspar deleted the noreturn-abort branch July 1, 2020 21:22
ndellingwood added a commit to ndellingwood/kokkos that referenced this pull request Aug 17, 2020
comment that guards added to workaround issues with cuda random num gen
with KOKKOS_ENABLE_DEBUG_BOUNDS_CHECK following changes from PR kokkos#3106
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve existing capability; will potentially require voting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants