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.

Sign up for GitHub

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

Jump to bottom

Change to make CudaMallocAsync available via opt-in with CMake option Kokkos_ENABLE_CUDAMALLOCASYNC #4233

Merged
merged 13 commits into from
Aug 27, 2021
Merged

Change to make CudaMallocAsync available via opt-in with CMake option Kokkos_ENABLE_CUDAMALLOCASYNC #4233

merged 13 commits into from
Aug 27, 2021

Conversation

matt-stack
Copy link
Contributor

#4228

Add the Kokkos_ENABLE_CUDAMALLOCASYNC option in CMake to enable cudaMallocAsync, OFF by default

@dalg24-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

masterleinad
masterleinad reviewed Aug 12, 2021
View reviewed changes
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

I would prefer CUDA_MALLOC_ASYNC over CUDA_MALLOCASYNC and you should be able to remove one preprocessor variable. We also might mention somewhere that this causes problems with UCX as of now.
Otherwise, this looks good to me.

cmake/KokkosCore_config.h.in Outdated Show resolved Hide resolved
cmake/kokkos_enable_options.cmake Outdated Show resolved Hide resolved
cmake/kokkos_enable_options.cmake Outdated Show resolved Hide resolved
core/src/Cuda/Kokkos_CudaSpace.cpp Outdated Show resolved Hide resolved
core/src/Cuda/Kokkos_CudaSpace.cpp Outdated Show resolved Hide resolved
@crtrott
Copy link
Member

crtrott commented Aug 12, 2021

Agree with Daniel regarding naming.

matt-stack and others added 5 commits August 12, 2021 10:55
@matt-stack @masterleinad
Update cmake/KokkosCore_config.h.in
9d79732 
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
@matt-stack @masterleinad
Update cmake/kokkos_enable_options.cmake
e416e00 
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
@matt-stack @masterleinad
Update cmake/kokkos_enable_options.cmake
6fa985e 
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
@matt-stack @masterleinad
Update core/src/Cuda/Kokkos_CudaSpace.cpp
593d8df 
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
@matt-stack @masterleinad
Update core/src/Cuda/Kokkos_CudaSpace.cpp
e1d9343 
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
@github-actions
Copy link

The Workflow run is cancelling this PR. It is an earlier duplicate of 3979050 run.

@github-actions
Copy link

The Workflow run is cancelling this PR. It is an earlier duplicate of 3979336 run.

@github-actions
Copy link

The Workflow run is cancelling this PR. It is an earlier duplicate of 3979336 run.

@github-actions
Copy link

The Workflow run is cancelling this PR. It is an earlier duplicate of 3979336 run.

matt-stack and others added 2 commits August 12, 2021 13:07
@matt-stack @masterleinad
Update cmake/kokkos_enable_options.cmake
a8f2015 
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
@matt-stack @masterleinad
Update cmake/kokkos_enable_options.cmake
401ffd4 
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
@dalg24
Copy link
Member

dalg24 commented Aug 13, 2021

We have been trying to contain the proliferation of CMake options. Why should we expose this one to the user?

dalg24
dalg24 requested changes Aug 13, 2021
View reviewed changes
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

#4228 mentions the issue reported will be resolved on the UCX side.
Users can use latest release version of Kokkos in the meantime. I do not see a strong rational to add a new option.

@masterleinad
Copy link
Contributor

Users can use latest release version of Kokkos in the meantime. I do not see a strong rational to add a new option.

For me, the situation is something like:

  • We wanted this feature since it improves performance.
  • Some MPI setups have problems with it and we don't have any control over it. My impression is that we can't even detect compatibility.

This implies to me that it should not be used by default, but people might still be interested in using it. That sounds like a good candidate for an experimental feature to me.

@crtrott
Copy link
Member

crtrott commented Aug 16, 2021

Agreeing with Daniel here, sorry Damien ...

@crtrott
Copy link
Member

crtrott commented Aug 16, 2021

Lets vote on Wednesday (while Damien is not here anyway :-) )

@dalg24
Copy link
Member

dalg24 commented Aug 16, 2021

OK to test

@crtrott crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Aug 25, 2021
@crtrott crtrott added this to In progress in Kokkos Release 3.5 via automation Aug 25, 2021
@crtrott crtrott moved this from In progress to Awaiting Feedback in Kokkos Release 3.5 Aug 25, 2021
dalg24
dalg24 reviewed Aug 26, 2021
View reviewed changes
cmake/KokkosCore_config.h.in Outdated
@@ -41,6 +41,7 @@
#cmakedefine KOKKOS_ENABLE_CUDA_LAMBDA
#cmakedefine KOKKOS_ENABLE_CUDA_CONSTEXPR
#cmakedefine KOKKOS_ENABLE_CUDA_LDG_INTRINSIC
#cmakedefine KOKKOS_ENABLE_CUDA_MALLOC_ASYNC
Copy link
Member

Choose a reason for hiding this comment

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

I forgot to add it to the agenda of yesterday's meeting but I will withdraw my objection if we mark the option as "experimental". Something like

Suggested change
#cmakedefine KOKKOS_ENABLE_CUDA_MALLOC_ASYNC
#cmakedefine KOKKOS_ENABLE_IMPL_CUDA_MALLOC_ASYNC

would do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

@crtrott
Copy link
Member

crtrott commented Aug 27, 2021

Retest this please.

@dalg24 dalg24 merged commit 32bcd26 into kokkos:develop Aug 27, 2021
Kokkos Release 3.5 automation moved this from Awaiting Feedback to Done Aug 27, 2021
@matt-stack matt-stack mentioned this pull request Aug 30, 2021
Merged
@masterleinad masterleinad mentioned this pull request Jul 1, 2023
Closed
@dalg24 dalg24 mentioned this pull request Sep 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 approved these changes

@masterleinad masterleinad masterleinad approved these changes

@crtrott crtrott crtrott approved these changes

Assignees
No one assigned
Labels
Blocks Promotion Overview issue for release-blocking bugs
Projects
Kokkos Release 3.5
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@matt-stack @dalg24-jenkins @crtrott @dalg24 @masterleinad