-
Notifications
You must be signed in to change notification settings - Fork 96
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
Support cmake versions below 3.24.0 #163
Conversation
…e/CMakeLists.txt to support cmake versions below 3.24
@@ -7,7 +7,9 @@ set(CMAKE_CUDA_STANDARD_REQUIRED TRUE) | |||
set(CMAKE_CXX_STANDARD_REQUIRED TRUE) | |||
# add the target cuda architectures | |||
# each additional architecture increases the compilation time and output file size | |||
if (NOT DEFINED CMAKE_CUDA_ARCHITECTURES) | |||
if (${CMAKE_VERSION} VERSION_LESS "3.24.0") | |||
set(CMAKE_CUDA_ARCHITECTURES ${CUDA_ARCH}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible that CUDA_ARCH
will be undefined ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it turns out that CUDA_ARCH
is undefined. When line 11 runs, however, CMAKE_CUDA_ARCHITECTURES
retains its original value. As such, perhaps a better way is:
if (${CMAKE_VERSION} VERSION_GREATER_EQUAL "3.24.0")
set(CMAKE_CUDA_ARCHITECTURES native)
endif ()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for now, this is good enough and I'll open an issue to ensure we made this robust
This PR modifies
icicle/CMakeLists.txt
to setCMAKE_CUDA_ARCHITECTURES
to${CUDA_ARCH}
if the cmake version is less than 3.24.0, andnative
otherwise. This change has been successfully tested with cmake 3.22.1 but I can't confirm if even earlier versions of cmake work.Linked Issues
Resolves #159. Thanks to @jeremyfelder for coming up with the solution!