Enable using Kokkos with CUDA backend for MIRCO and in general - #2012
Enable using Kokkos with CUDA backend for MIRCO and in general#2012PhilipOesterlePekrun wants to merge 15 commits into
Conversation
Signed-off-by: Philip Oesterle-Pekrun <philipoesterlepekrun@gmail.com>
da5207d to
575c3b3
Compare
|
@PhilipOesterlePekrun Thanks for the work! I really like the efforts, yet I also have some concerns. The current PR comes with quite a few assumptions and restrictions we should clearly state and discuss (maybe something for the next community meeting?). |
|
Concerning your questions:
For my personal taste, the current state has too many constraints to get things to work. Are there parts we can work on separately to make the use of CUDA easier in the near future? |
|
@maxfirmbach Thanks for the feedback. I tried to keep it as isolated as possible, but yes it does change some global CMake files (though everything is conditional upon the global option I introduced). We can definitely discuss this in the community meeting (or even earlier). Regarding your second comment, the constraint here is really that GCC cannot compile CUDA-related code but the I do have a general question, @isteinbrecher and @maxfirmbach, to clarify the purpose of my PR. |
|
@PhilipOesterlePekrun To my knowledge, nobody ever ran 4C on device so far. |
There was a problem hiding this comment.
Pull request overview
This PR adds build-system support to compile 4C with a CUDA-enabled Kokkos installation by introducing a clangcuda++ compiler wrapper and a FOUR_C_CLANGCUDA CMake option that adjusts compile definitions/launchers for clang-based CUDA host/device compilation. It also updates ArborX/Kokkos usage to explicitly run on the host when Kokkos’ default execution space may be CUDA, and refreshes MIRCO integration settings.
Changes:
- Added
utilities/clangcuda++wrapper to drive clang CUDA host-only/device compilation based on compile definitions and file extensions. - Introduced
FOUR_C_CLANGCUDAglobal option and applied related target/global launcher settings +FOUR_C_CLANGCUDA_HOST_ONLYcompile definition in key build targets. - Switched geometric search ArborX execution/memory spaces to
Kokkos::DefaultHostExecutionSpaceto avoid unintended CUDA default execution space usage; updated MIRCO fetch configuration and git tag.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| utilities/clangcuda++ | New clang CUDA compiler wrapper handling host-only/device compilation modes. |
| src/cut/4C_cut_pointgraph.cpp | Adds a clang-CUDA-related preprocessor workaround before including Boost graphviz. |
| src/core/geometric_search/src/4C_geometric_search_distributed_tree.cpp | Forces ArborX distributed tree build/query to use host execution space. |
| src/core/geometric_search/src/4C_geometric_search_bvh.hpp | Forces BVH execution/memory space selection to host execution space. |
| cmake/setup_global_options.cmake | Adds FOUR_C_CLANGCUDA global option. |
| cmake/functions/four_c_auto_define_module.cmake | Applies launcher overrides + host-only define to module object libraries when enabled. |
| cmake/configure/configure_Trilinos.cmake | Disables compiler/rule launchers when FOUR_C_CLANGCUDA is enabled. |
| cmake/configure/configure_MIRCO.cmake | Updates MIRCO fetch configuration and moves dependency registration outside the conditional. |
| apps/global_full/CMakeLists.txt | Applies launcher overrides + host-only define to the main executable when enabled. |
@PhilipOesterlePekrun Understood! I also don't think that someone has tried this so far, because there was frankly no reason for it ... all of the code in |
5332d8c to
3d2f036
Compare
|
@maxfirmbach A built test is possible, however this requires the following:
Again, the current changes are isolated with By the way, an actual run test is of course not possible unless we have GPU test runners for the workflow/action. |
Signed-off-by: Philip Oesterle-Pekrun <philipoesterlepekrun@gmail.com>
|
@PhilipOesterlePekrun Updating the Dockerfile should be fine. Especially due to the specialized nature of the Kokkos-Cuda integration of MIRCO into 4C, where the number of users and experts is limited to less than a handful, proper testing is really important. So, I'd support to change the docker base containers and provide Cuda with them. |
Just a heads up, it might not be possible to install the cuda packages into the normal Docker image we use for testing due to the size of cuda. In the past, we had problems that there was not enough space left on the runner (14 GB disk space) to build 4C because our docker image is so big. So, we need to be careful what we add to the dependency image. Doing Maybe there is a way to install only a minimal version of cuda that is sufficient for the pipeline. Or you can try to create a separate docker image that only has the minimum dependencies to build 4C with Cuda, e.g., the |
|
@ppraegla Yes, a separate DockerFile like the one for trilinos_develop is the way to go. I do have it with the minimal cuda toolkit requirements, which includes cusolver, but it still adds a few GB so I don't want to affect the main 4C docker image. |
|
We now also briefly discussed after the meeting, and I (in my opinion) still strongly oppose adding a lot of new functionality without testing it. My main question would be: why is then the docker container even created in the first place if it's not used for anything afterwards? If you use this workflow daily in the IMCS you will probably not download the docker container but install it on your local system. So why creating the docker container when it won't be tested at all? @PhilipOesterlePekrun @4C-multiphysics/maintainer |
|
@davidrudlstorfer The compilation is tested, but it's true that this may not be necessary until we have Kokkos device code in some other parts of 4C, as the compilation of MIRCO itself is already tested in MIRCO's repository. So, if we agree that it is not needed currently, I can keep those changes in a branch of my fork in case we do want it later, that is totally fine too. The docker would have just been for the build test, yes. |
|
@mayrmt I've attached the full build.log which I made with build.log.gz (compressed due to GitHub file size limit) |
3d2f036 to
6da5580
Compare
Signed-off-by: Philip Oesterle-Pekrun <philipoesterlepekrun@gmail.com>
|
Fantastic, then the user just needs to get the correct flags sent to Kokkos via building Trilinos correctly. One point of clarification
I had to set all four of |
3845f45 to
779d5b9
Compare
|
Yes I fixed the documentation to be more clear. I think we always use MPI because it is a required "TPL". Lots of retesting, but I think it is good now and cleaner than before, what do you think @mayrmt ? |
Signed-off-by: PhilipOesterlePekrun <philipoesterlepekrun@gmail.com>
Signed-off-by: PhilipOesterlePekrun <philipoesterlepekrun@gmail.com>
Signed-off-by: PhilipOesterlePekrun <philipoesterlepekrun@gmail.com>
36814a5 to
d107115
Compare
|
Small edit to the docker workflow, which I added to a past commit. Anyways, I think we can merge at this point? |
|
ok, in my PR here, which looks good now, I add the patch release of Trilinos as a supported version |
|
Note: With this branch and #2121 I still need the following change to compile locally $ git diff
diff --git a/cmake/functions/four_c_auto_define_benchmark_tests.cmake b/cmake/functions/four_c_auto_define_benchmark_tests.cmake
index 8af9b98914..60eb30bff9 100644
--- a/cmake/functions/four_c_auto_define_benchmark_tests.cmake
+++ b/cmake/functions/four_c_auto_define_benchmark_tests.cmake
@@ -31,7 +31,7 @@ function(_set_up_benchmark_test_target _module_under_test _target)
target_link_libraries(${_target} PRIVATE unittests_common)
if(FOUR_C_CLANGCUDA)
- set_clangcuda_mode(${_target} CLANGCUDA_MODE_DEVICE)
+ set_clangcuda_mode(${_target} CLANGCUDA_MODE_HOST)
endif()
# configure the respective input test with the respective mesh |
|
@jeremylt Yes, I've changed the Trilinos install scripts in this PR to use 16.2.1 as well, and I've changed the benchmark tests to CLANGCUDA_MODE_HOST. Just waiting on the docker image to be updated, as I am not a maintainer and cannot do it. |
|
Sounds good, just wanted to check in since I merged your branch into mine for testing purposes |
|
@PhilipOesterlePekrun I just have updated the docker images per your instructions. Please let me know, if that has worked. |
|
Need one more approval to merge :) |
Signed-off-by: Philip Oesterle-Pekrun <philipoesterlepekrun@gmail.com>
Co-authored-by: Jeremy L Thompson <jeremy@jeremylt.org> Signed-off-by: Philip Oesterle-Pekrun <philipoesterlepekrun@gmail.com>
Co-authored-by: Jeremy L Thompson <jeremy@jeremylt.org> Signed-off-by: Philip Oesterle-Pekrun <philipoesterlepekrun@gmail.com>
e25ad6f to
20b6d46
Compare
|
The dependencies hash computed after auto-merge differs, because the computation wasn't isolated enough. I'll have to rebuild the docker also because I needed to update the hash for openmp Trilinos. |
@mayrmt
Description and Context
This PR adds build-system support for compiling 4C with CUDA-enabled Kokkos. For this purpose, this PR introduces a compiler wrapper,
clangcuda++, which should be used as theCMAKE_CXX_COMPILER(or, in the case of MPI, theOMPI_CXXbackend), as well as some CMake additions which are optionally enabled.This change is based on my attempts to get 4C to compile while using MIRCO (library) with Kokkos' CUDA backend for GPU offloading. Several 4C translation units fail to compile with the
kokkos_launch_compilerornvcc_wrapperprovided by Kokkos because NVCC does not seem to be able to compile some more "advanced" C++ code, which we have a fair bit of in 4C since it is a large project. Clang happens to compile CUDA or CUDA-related code much better than Nvidia's own compiler (don't ask me why), but still has some issues by itself. Using theclangcuda++wrapper as theCMAKE_CXX_COMPILER(or, in the case of MPI, theOMPI_CXXbackend), along with the corresponding target properties and compile definitions by settingFOUR_C_CLANGCUDA=ON, allows compiling all of 4C with CUDA-enabled Kokkos.The current implementation distinguishes between CUDA host-side compilation and CUDA device compilation. At the moment, 4C itself only requires CUDA host-side compilation, since it does not yet contain raw Kokkos device kernels such as
Kokkos::parallel_for,KOKKOS_LAMBDA, etc. in its own sources. But, this is now possible by simply marking a target or source withFOUR_C_CLANGCUDA_DEVICE_COMPILE, allowing GPU offloading anywhere in 4C. As long as Trilinos is built withTPETRA_INST_CUDA=OFF, this also does not conflict with any existing MPI parallelism (NOwill be KokkosSerial).The changes were verified by compiling 4C successfully with the relevant Kokkos CUDA-enabled setup and I also tested small Kokkos kernel examples to confirm that host-side and device CUDA compilation are distinguished correctly. I have documented how Trilinos, MIRCO, and 4C should be compiled for this combination to work. I'll just attach that to this PR for now: DocumentKokkosCuda_4C_Trilinos_MIRCO.zip
Once imcs-compsim/MIRCO#146 is merged, FetchContent will work without issue for that MIRCO state.
My questions:
Related Issues and Pull Requests
Blocked by imcs-compsim/MIRCO#146
Docker and Workflow tests
I have added a Dockerfile, Trilinos installation scripts, and the buildtest and docker workflows, which are similar to the main buildtest.yaml and docker.yaml.