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

Compile as cmake language #555

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JBludau
Copy link
Contributor

@JBludau JBludau commented Jul 26, 2024

Some documentation for Kokkos requiring to be built with the CMake language feature. Since kokkos_compilation since kokkos_compilation is broken see here for a momentary fix.

7582 is a related refactor of the build system but is independent of this PR

@masterleinad masterleinad requested a review from ajpowelsnl July 29, 2024 18:59
@masterleinad
Copy link
Contributor

Looks like we need something like

diff --git a/cmake/KokkosConfigCommon.cmake.in b/cmake/KokkosConfigCommon.cmake.in
index d3ac39ffa..fe021199e 100644
--- a/cmake/KokkosConfigCommon.cmake.in
+++ b/cmake/KokkosConfigCommon.cmake.in
@@ -33,6 +33,9 @@ ENDFOREACH()
 
 IF(Kokkos_ENABLE_CUDA)
   SET(Kokkos_CUDA_ARCHITECTURES @KOKKOS_CUDA_ARCHITECTURES@)
+  IF(Kokkos_ENABLE_COMPILE_AS_CMAKE_LANGUAGE)
+    SET(CMAKE_CUDA_ARCHITECTURES @KOKKOS_CUDA_ARCHITECTURES@)
+  ENDIF()
 ENDIF()
 
 IF(Kokkos_ENABLE_HIP)
@@ -300,8 +303,8 @@ FUNCTION(kokkos_compilation)
             # set the properties if defined
             IF(COMP_${_TYPE})
                 # MESSAGE(STATUS "Using ${COMP_COMPILER} :: ${_TYPE} :: ${COMP_${_TYPE}}")
-                SET_PROPERTY(${_TYPE} ${COMP_${_TYPE}} PROPERTY RULE_LAUNCH_COMPILE "${Kokkos_COMPILE_LAUNCHER} ${COMP_COMPILER} ${CMAKE_CXX_COMPILER}")
-                SET_PROPERTY(${_TYPE} ${COMP_${_TYPE}} PROPERTY RULE_LAUNCH_LINK "${Kokkos_COMPILE_LAUNCHER} ${COMP_COMPILER} ${CMAKE_CXX_COMPILER}")
+                SET_PROPERTY(${_TYPE} ${COMP_${_TYPE}} PROPERTY LANGUAGE ${Kokkos_COMPILE_LANGUAGE})
+                SET_PROPERTY(${_TYPE} ${COMP_${_TYPE}} PROPERTY CUDA_ARCHITECTURES ${Kokkos_CUDA_ARCHITECTURES})
             ENDIF()
         ENDFOREACH()
     ENDIF()

for this to be usable with Kokkos_ENABLE_COMPILE_AS_CMAKE_LANGUAGE.

@masterleinad
Copy link
Contributor

The challenge is basically that we should set CUDA_ARCHITECTURES for every target that is to be compiled with Kokkos (and we can't set the property for a source file). On the other hand, we can only set the compile language for a source file, not for a target and we can't really iterate over the source files of a target in general. That means we have to use kokkos_compilation on all the source files and we must find Kokkos before declaring any targets (or also use kokkos_compilation on all targets).

@ajpowelsnl
Copy link
Contributor

ajpowelsnl commented Jul 30, 2024

@cedricchevalier19 - this entry is relevant to 7144, CMAKE_AS_LANGUAGE and kokkos_compilation()

@dalg24
Copy link
Member

dalg24 commented Jul 30, 2024

@cedricchevalier19 - this entry is possibly relevant to 7144, CMAKE_AS_LANGUAGE and kokkos_compilation()

Jakob clearly states in the description that this work is a proposed resolution for that very issue so yes it is (hopefully) relevant

@masterleinad
Copy link
Contributor

kokkoc_compilation in its current form only sets the launch compiler. Hence, it's not useful with Kokkos_ENABLE_COMPILE_AS_CMAKE_LANGUAGE=ON. I think we should just document the current behavior for now.

docs/source/building.md Outdated Show resolved Hide resolved
docs/source/building.md Outdated Show resolved Hide resolved
@JBludau
Copy link
Contributor Author

JBludau commented Jul 31, 2024

kokkoc_compilation in its current form only sets the launch compiler. Hence, it's not useful with Kokkos_ENABLE_COMPILE_AS_CMAKE_LANGUAGE=ON. I think we should just document the current behavior for now.

I agree with your assessment, I ended at the same places in the cmake files.
Nevertheless, I would vote for trying to support this feature, even if the user needs to annotate both the source and target. HIP seems to recommend the use of the CMake language feature (see first Attention box)

@masterleinad
Copy link
Contributor

Nevertheless, I would vote for trying to support this feature, even if the user needs to annotate both the source and target.

Sure. I was just suggesting a more incremental approach. First, document the current behavior of kokkos_compilation, then try to accommodate it when using Kokkos_ENABLE_COMPILE_AS_CMAKE_LANGUAGE (or even define a different function since the behavior needed is very different), and finally document it.

@masterleinad
Copy link
Contributor

HIP seems to recommend the use of the CMake language feature (see first Attention box)

Kokkos just works better when not enabling HIP (or CUDA) as a feature, though.

docs/source/building.md Outdated Show resolved Hide resolved
@JBludau JBludau force-pushed the compile_as_cmake_language branch from 5c04488 to 297f684 Compare January 11, 2025 00:04
@JBludau JBludau marked this pull request as ready for review January 11, 2025 00:08
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.

6 participants