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

use CMake builtin mechanisms for setting target CUDA architecture(s) #122

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

jameslamb
Copy link
Member

Contributes to #115

As described at https://cmake.org/cmake/help/latest/variable/CMAKE_CUDA_ARCHITECTURES.html, if the target GPU architectures isn't explicitly provided to CMake, it falls back to a concrete (but system-and-compiler-specific) version.

To help with that, #81 introduced two things:

  • ability to set the target architecture via an environment variable CUDA_ARCH
  • fallback to native (whatever GPU is detected on the system where the build is running)

This proposes preserving that functionality, but in a different way:

  • using the environment variable CUDAARCHS, which CMake has supported since v3.18 (docs link), instead of introducing a custom one
  • ensuring that architecture can also be overridden for pip install

Notes for Reviewers

Benefits of these changes

Reduces complexity in managing CUDA architecture, by reducing the number of paths through which configuration flows to CMake. This will be important when we introduce yet another build pattern (conda builds) here.

How I tested this

Added code like this at the bottom of the top-level CMakeLists.txt.

get_target_property(
  REQUESTED_ARCHITECTURES
  legateboost
  CUDA_ARCHITECTURES
)

message(FATAL_ERROR "requested architectures: ${REQUESTED_ARCHITECTURES}")

Found the expected behavior for regular CMake builds, build.sh, and pip install.

@jameslamb jameslamb added breaking Introduces a breaking change improvement Improves an existing functionality labels Jul 18, 2024
@jameslamb jameslamb requested a review from RAMitchell July 18, 2024 19:21
@jameslamb jameslamb changed the title WIP: use CMake builtin mechanisms for setting target CUDA architecture(s) use CMake builtin mechanisms for setting target CUDA architecture(s) Jul 18, 2024
@jameslamb jameslamb requested a review from trivialfis July 18, 2024 19:55
@jameslamb jameslamb marked this pull request as ready for review July 18, 2024 19:55
@jameslamb
Copy link
Member Author

I especially want your opinion on this @trivialfis, to be sure I correctly understood the goals of #81

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Looks good to me! The PR I created was to prefer the CMake standard argument over the original custom argument. Anything that removes magic is welcome!

@jameslamb
Copy link
Member Author

Great, thank you!

@jameslamb jameslamb merged commit 2f2a6ee into rapidsai:main Jul 19, 2024
3 checks passed
@jameslamb jameslamb deleted the cuda-arch branch July 19, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces a breaking change improvement Improves an existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants