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

Replace use of FindCUDA with native CMake CUDA support. #154

Merged
merged 6 commits into from
May 17, 2018

Conversation

robinson96
Copy link
Contributor

Meant to be a drop in replacement for FindCUDA. The only change I had to do do my host-config was to drop an explicit definition of -std=c++, as the native CMake puts it in there the right way and nvcc complains if you define it twice.

Copy link
Member

@cyrush cyrush left a comment

Choose a reason for hiding this comment

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

If this works for your use cases, I think its safe to transition. What is the minimum version of CMake this will require - 3.9 or 3.10? We should make a note in the docs about that.

@davidbeckingsale
Copy link
Member

How does this handle stuff that only needs the CUDA runtime library?

@robinson96
Copy link
Contributor Author

@cyrush : I'm using 3.9.2 - have not tested with earlier versions.
@davidbeckingsale : I think I have something that in theory should exercise that, but will confirm for you. Might be nice if you had an existing project for me to clone and test this with.

if (ENABLE_CUDA)
enable_language(CUDA)
# where to find nvcc
if(CUDA_BIN_PATH)
Copy link
Member

Choose a reason for hiding this comment

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

Please remove support for as many FindCUDA variables as possible. I would like this as close to normal CMake conventions as possible. This reduces the amount of upkeep we need to support going forward. This applies to CUDA_BIN_PATH, CUDA_HOST_COMPILER, and CUDA_NVCC_FLAGS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding CUDA_SEPARABLE_COMPILATION - I think it's best practice to do this in general, I don't think there is harm to doing separable compilation but it's certainly a pitfall folks can encounter. Should I just do that by default for the sake of killing the CUDA_SEPARABLE_COMPILATION flag?

Copy link
Member

Choose a reason for hiding this comment

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

I think users can easily control globally withCMAKE_CUDA_SEPARABLE_COMPILATION -- we don't have any per-target options for separable compilation exposed in the macro sig -- so I think you can at least pull it out from this macro.

Copy link
Member

@davidbeckingsale davidbeckingsale left a comment

Choose a reason for hiding this comment

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

I want to verify that we still have the "cuda_runtime" dependency working

@white238
Copy link
Member

The existing CUDA examples will need to be switched as well as documentation updated. I can help with any of that if you would like.

Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

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

Thanks @robinson96.

Have you tested this update against blt's cuda examples? Please ensure that the cuda-based tests and examples pass under this PR, and update the host-config for ray as necessary (in the host-configs directory).

Note: The old host-config for ray was broken the last time I checked (see #146).

@cyrush cyrush mentioned this pull request May 15, 2018
@robinson96
Copy link
Contributor Author

@davidbeckingsale : we are still using the CMAKE built in FindCUDA to set all the libraries and paths necessary for the cuda_runtime target. I've verified that that behavior is reasonable.

@davidbeckingsale
Copy link
Member

davidbeckingsale commented May 16, 2018 via email

@robinson96
Copy link
Contributor Author

rzmanta32{probinso}24: make test
Running tests...
Test project /g/g18/probinso/git/blt/tests/internal/build
Start 1: t_example_smoke
1/7 Test #1: t_example_smoke .................. Passed 0.01 sec
Start 2: t_header_only_smoke
2/7 Test #2: t_header_only_smoke .............. Passed 0.01 sec
Start 3: blt_gtest_smoke
3/7 Test #3: blt_gtest_smoke .................. Passed 0.01 sec
Start 4: blt_mpi_smoke
4/7 Test #4: blt_mpi_smoke .................... Passed 0.95 sec
Start 5: blt_cuda_smoke
5/7 Test #5: blt_cuda_smoke ................... Passed 0.13 sec
Start 6: blt_cuda_runtime_smoke
6/7 Test #6: blt_cuda_runtime_smoke ........... Passed 0.03 sec
Start 7: t_cuda_device_call_from_kernel
7/7 Test #7: t_cuda_device_call_from_kernel ... Passed 0.12 sec

100% tests passed, 0 tests failed out of 7

Total Test time (real) = 1.31 sec
rzmanta32{probinso}25:

@rhornung67
Copy link
Member

@davidbeckingsale and @robinson96 you may want to check with @ajkunen about CMake versions. I recall him telling me that he had issues with some later versions.

@cyrush
Copy link
Member

cyrush commented May 16, 2018

@rhornung67 thanks for the warning, that rings with what I fear.
I am hoping we can stick with 3.9.x.

@rhornung67
Copy link
Member

@cyrush my purpose is to ring your fears....

@ajkunen
Copy link

ajkunen commented May 16, 2018 via email

@davidbeckingsale
Copy link
Member

Hmm, I'm sure you were doing something stupid 😄 did you ever try with a version > 3.9.2?

@ajkunen
Copy link

ajkunen commented May 16, 2018 via email

@kennyweiss
Copy link
Member

Thanks for fixing the host-configs @robinson96.
Please close #146 when you merge.

@robinson96
Copy link
Contributor Author

FWIW: the surface test I ran under this stuff was using cmake 3.8. That doesn't exercise the LINK_WITH_NVCC stuff, but it does exercise the SEPARABLE_COMPILATION.

@robinson96 robinson96 merged commit 8c74699 into master May 17, 2018
@kennyweiss kennyweiss deleted the feature/probinso/cmakecuda branch April 23, 2019 22:41
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.

8 participants