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

CMake: Only show GPU architectures with enabled corresponding backend #5119

Merged
merged 5 commits into from
Jun 17, 2022

Conversation

masterleinad
Copy link
Contributor

Following a discussion in the meeting yesterday, we use cmake_dependent_option for the architecture options to only show GPU architectures in ccmake if the corresponding backend is enabled (or we configure with Kokkos_ENABLE_UNSUPPORTED_ARCHS=ON). This should make the CMake option a little less overwhelming.

@crtrott
Copy link
Member

crtrott commented Jun 16, 2022

How does this work with SYCL on NVIDIA?

cmake/kokkos_arch.cmake Show resolved Hide resolved
cmake/kokkos_arch.cmake Outdated Show resolved Hide resolved
@masterleinad
Copy link
Contributor Author

How does this work with SYCL on NVIDIA?

You have to use Kokkos_ENABLE_UNSUPPORTED_ARCHS=ON anyway and that enables KOKKOS_SHOW_CUDA_ARCHS so that the CUDA architectures are shown.

@crtrott
Copy link
Member

crtrott commented Jun 16, 2022

ok sounds good

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.

I would prefer TRUE -> ON

Copy link
Contributor

@JBludau JBludau 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

Copy link
Contributor

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

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

the cmake_parse_arguments suggestion is minor and don't mind if it's not adopted. I think adding the FORCE parameter is important for dependent options because we may want other force values than OFF

LIST(APPEND KOKKOS_OPTION_TYPES BOOL)
SET(KOKKOS_OPTION_TYPES ${KOKKOS_OPTION_TYPES} PARENT_SCOPE)

CMAKE_DEPENDENT_OPTION(${CAMEL_NAME} ${DOCSTRING} ${DEFAULT} "${DEPENDENCY}" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should have a FORCE argument so the api is the same as cmake_dependent_option

cmake/kokkos_arch.cmake Show resolved Hide resolved
@@ -57,7 +57,46 @@ FUNCTION(kokkos_option CAMEL_SUFFIX DEFAULT TYPE DOCSTRING)
# Make sure this appears in the cache with the appropriate DOCSTRING
SET(${CAMEL_NAME} ${DEFAULT} CACHE ${TYPE} ${DOCSTRING})

#I don't love doing it this way because it's N^2 in number options, but cest la vie
#I don't love doing it this way because it's N^2 in number options, but c'est la vie
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for adding a fix to the missing ' it was really bugging me :P

@crtrott crtrott merged commit ae496eb into kokkos:develop Jun 17, 2022
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.

None yet

5 participants