Add support for Kokkos v5, with multi-backend device support. - #418
Conversation
abd4311 to
f68b1e7
Compare
|
@lightsighter the main issues I'm having building with Legion/Realm/Kokkos 5 all are now about CUDA 12.9/13.3, GCC 14+, LLVM 20/21/22+ and C++20. I have yet to find the magic combo that works. |
|
@lightsighter @mpokorny I've looked at this closer. The issues I'm seeing come from the inclusion of |
|
@lightsighter See #462 for my additional patch. It requires CMake changes to avoid dealing with the Kokkos headers directly. |
|
It just so happens that I've been working on this this week, too. I will try to update this branch today. |
This commit drops the support for Kokkos below v4, and it also adds support for multiple backend Kokkos devices of any type in a Legion process.
Add OpenMP::OpenMP_CXX to REALM_PUBLIC_LIBRARIES when using the system OpenMP. Also add the get_num_threads() function to the LocalOpenMPProcessor.
|
I've updated this branch with the latest patch that I've been developing and using. I've included the multi-backend device capability because it's useful, I'm getting tired of maintaining it separately, and I'm feeling lazy. However, if people object, I will separate out those changes. |
|
@mpokorny in case you looked at my commit, look again please. My late night commit was missing the important part 😁 |
|
I'll test out this updated branch. I'll prefer this simpler solution if we can avoid the Kokkos::Array parts entirely. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #418 +/- ##
==========================================
- Coverage 30.41% 30.41% -0.01%
==========================================
Files 199 199
Lines 41281 41282 +1
Branches 14942 14758 -184
==========================================
Hits 12554 12554
+ Misses 28272 27279 -993
- Partials 455 1449 +994 ☔ View full report in Codecov by Harness. |
I think these changes look straight-forward enough that we can evaluate them all in the same pull request. They also related since you couldn't do some of these things until Kokkos 5 anyway right? |
|
That's correct. I'm still actively testing this at the moment. I'll report my findings and/or update the PR as needed, and provide an indication when I think all is OK. |
|
This branch is passing all my tests for the radio camera imaging pipeline application (plus its unit tests) with multiple processes and >=1 GPU per process. |
|
@lightsighter this still needs to get merged in for us. |
|
@mpokorny Are you ready to merge this? at a minimum you need to run the formatting command from the Realm root directory which should resolve the formatting issue in the CI: If you can't get it to work then I can do it for you. |
|
I applied the |
|
It looks like the format is still unhappy. Maybe you're on a different clang-format version? Looks like we use LLVM 15. Alternatively you could use the diff printed in the CI job: https://github.com/StanfordLegion/realm/actions/runs/32170123110/job/95819562500?pr=418 |
|
@mpokorny Is there a Legion-side change that needs to go along with this when we update Realm there? |
|
Yes, |
|
That or send a patch. I'll probably want to incorporate it into the Realm update MR so that it lands together. |
|
Does this work? |
|
This has now been merged into Legion along with the example patch. Thanks @mpokorny! |
Kokkos_Array.hppis included ininst_layout.hto get the correct template parameters in theKokkos::Arraydeclaration. It's not quite as simple as declaring the two-parameter version because the correct choice depends on whether or not v4 deprecated code is enabled in Kokkos.I tested the build with both Kokkos v4.7 and v5.0, but there are not unit tests in Realm to verify the result. I did, however, build Legion with this branch against both Kokkos versions, and verified the results using
kokkos_saxpy.