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

RMM doesn't require the CUDA language to be enabled by consumers #737

Conversation

robertmaynard
Copy link
Contributor

This allows consumers of libraries such as cudf that offer a C++ interface to not require the CUDA language be enabled.

This allows consumers of libraries such as `cudf` that offer a C++
interface to not require the CUDA language be enabled.
@kkraus14 kkraus14 added bug Something isn't working non-breaking Non-breaking change labels Mar 24, 2021
@kkraus14
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 1de6b83 into rapidsai:branch-0.19 Mar 24, 2021
@robertmaynard robertmaynard deleted the fix/remove_requirement_of_cuda_language_for_rmm_consuming_projects branch March 31, 2021 13:11
@gnaggnoyil
Copy link

I'm sorry, but if the goal is "to allow consuming targets that offers C++ interface only not requiring CUDA enabled", wouldn't it be a better solution to let the consuming target link rmm as private interface instead? The change here seems to indicate that rmm::rmm requires a C++ standard but doesn't require any CUDA ones, which seems not to fit the goal very well I think.

@jrhemstad
Copy link
Contributor

I'm sorry, but if the goal is "to allow consuming targets that offers C++ interface only not requiring CUDA enabled", wouldn't it be a better solution to let the consuming target link rmm as private interface instead? The change here seems to indicate that rmm::rmm requires a C++ standard but doesn't require any CUDA ones, which seems not to fit the goal very well I think.

I'm not sure what precipitated this change, but strictly speaking, by design, RMM doesn't have any CUDA device code in its headers. It obviously has CUDA runtime calls like cudaMalloc, but no kernel launches nor device-side code that would require compiling with nvcc.

As such, I don't think it needs to require the CUDA language enabled, but just have the rmm::rmm target link against cudart. That said, I'm pretty willfully ignorant on build issues like this, so I may be wildly off base.

@gnaggnoyil
Copy link

I see. That makes sense. Thanks for your explanation!

@robertmaynard
Copy link
Contributor Author

As such, I don't think it needs to require the CUDA language enabled, but just have the rmm::rmm target link against cudart. That said, I'm pretty willfully ignorant on build issues like this, so I may be wildly off base.

You are correct. The reason for this change is that before CMake 3.22 stating target_compile_features(X cuda_std_14) would error out if the consuming project didn't have the CUDA language enabled. This was an issue for RMM C++ consumers which didn't shouldn't need the CUDA language enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CMake non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants