C++20 module for Kokkos std algorithms and basic setup #8132
Conversation
4c95efe to
cf31d99
Compare
cf31d99 to
2e335b5
Compare
2e335b5 to
f218cdb
Compare
Signed-off-by: Daniel Arndt <arndtd@ornl.gov>
7e02fa7 to
aed24a0
Compare
|
I am interested in the algorithms section of Kokkos and I feel like taking a look. I am wondering why sort and random are left out? |
I was going to do them later since they either use configuration macros or have more complicated dependencies on other parts of the library (like |
|
What do others do:
https://github.com/hanickadot/compile-time-regular-expressions:
https://github.com/fmtlib/fmt:
https://github.com/llvm/llvm-project/tree/main/libcxx/modules/std:
|
…ure-time Signed-off-by: Daniel Arndt <arndtd@ornl.gov>
108b5ad to
c022176
Compare
|
The last commit c022176 provides an alternative to modifying the test files directly by copying them to the build directory and then replacing includes with imports for headers that have a corresponding module. |
algorithms/unit_tests/CMakeLists.txt
Outdated
| endif() | ||
| endforeach() | ||
| get_filename_component(FILE_NAME ${FILE} NAME) | ||
| file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/${FILE_NAME}" "${HEADER_CONTENT}") |
There was a problem hiding this comment.
in case of an install ... would this still work?
There was a problem hiding this comment.
We don't install test headers if that's what you mean.
There was a problem hiding this comment.
Algorithms seem easier to modularize indeed.
I have concerns about the test headers that have to be duplicated and modified by CMake. I think this may be avoided.
What would be the problem of having the export in the source files directly (protected by macros, of course)? (Rather than making an explicit listing, or modifying the source files on-the-fly like in #8117.) I'm not sure about the best way to expose the modules.
I'm interested to see where this is going.
algorithms/src/CMakeLists.txt
Outdated
| @@ -9,6 +9,8 @@ file(GLOB ALGO_SOURCES *.cpp) | |||
| append_glob(ALGO_HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/std_algorithms/*.hpp) | |||
| append_glob(ALGO_HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/std_algorithms/impl/*.hpp) | |||
|
|
|||
| set(KOKKOS_ALGORITHMS_MODULE_FILES Kokkos_StdAlgorithms.ccm Kokkos_StdAlgorithmsImpl.ccm) | |||
There was a problem hiding this comment.
Do you need to prefix this variable, as it is internal? To reflect ALGO_HEADERS and ALGO_SOURCES, I'd propose ALGO_MODULES.
There was a problem hiding this comment.
That's fine with me.
algorithms/unit_tests/CMakeLists.txt
Outdated
| foreach(FILE ${TEST_HEADERS}) | ||
| file(READ ${FILE} HEADER_CONTENT) | ||
| foreach(PAIR IN LISTS MODULE_MAP) | ||
| if(PAIR MATCHES "^([^:]+):(.*)$") | ||
| set(HEADER "${CMAKE_MATCH_1}") | ||
| set(MODULE "${CMAKE_MATCH_2}") | ||
| string(REPLACE "${HEADER}" "#include <Kokkos_Macros.hpp>\n${MODULE};\n${MODULE}_impl;" HEADER_CONTENT | ||
| "${HEADER_CONTENT}" | ||
| ) | ||
| endif() | ||
| endforeach() | ||
| get_filename_component(FILE_NAME ${FILE} NAME) | ||
| file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/${FILE_NAME}" "${HEADER_CONTENT}") |
There was a problem hiding this comment.
If I understand correctly, you duplicate all test header files and replace the #include ... by import ..., but why not modify the test header files just like you modified the test files?
This is better than modifying files in the source tree on-the-fly, but I'm still wondering if this is really needed.
There was a problem hiding this comment.
We can't really modify files in the source tree anyway with our current CMake settings.
That being said, I would rather have the modifications to use modules instead of includes directly in the source and test files (than generating files in the build tree).
| namespace Kokkos::Experimental::Impl { | ||
| using ::Kokkos::Experimental::Impl::are_iterators_v; | ||
| using ::Kokkos::Experimental::Impl::are_random_access_iterators_v; | ||
| using ::Kokkos::Experimental::Impl:: | ||
| ExclusiveScanDefaultFunctorWithValueWrapper; | ||
| using ::Kokkos::Experimental::Impl:: | ||
| ExeSpaceTransformInclusiveScanNoInitValueFunctor; | ||
| using ::Kokkos::Experimental::Impl:: | ||
| ExeSpaceTransformInclusiveScanWithInitValueFunctor; | ||
| using ::Kokkos::Experimental::Impl::expect_no_overlap; | ||
| using ::Kokkos::Experimental::Impl::InclusiveScanDefaultFunctor; | ||
| using ::Kokkos::Experimental::Impl::is_admissible_to_kokkos_std_algorithms; | ||
| using ::Kokkos::Experimental::Impl::RandomAccessIterator; | ||
| using ::Kokkos::Experimental::Impl:: | ||
| static_assert_is_admissible_to_kokkos_std_algorithms; | ||
| using ::Kokkos::Experimental::Impl::StdAlgoLessThanBinaryPredicate; | ||
| using ::Kokkos::Experimental::Impl:: | ||
| StdNumericScanIdentityReferenceUnaryFunctor; | ||
| using ::Kokkos::Experimental::Impl:: | ||
| TransformExclusiveScanFunctorWithValueWrapper; | ||
| using ::Kokkos::Experimental::Impl::ValueWrapperForNoNeutralElement; | ||
| } // namespace Kokkos::Experimental::Impl |
There was a problem hiding this comment.
Do you think there are some Impl symbols that should be on the public API?
There was a problem hiding this comment.
No, not here. If we need to expose them to the user, they shouldn't be in Impl.
I recognize that we have some symbols in core that are used as customization points, though, and we will have to expose them (until they are moved out of the Impl namespace).
There was a problem hiding this comment.
I mean, it's a good timing to "promote" some of them that may be exposed, on second thought, if any.
I presented to approaches and prefer the first one that modifies the test files directly instead of generating new files. @dalg24 preferred to minimize changes to existing files and so I prototyped an alternative.
We can do that but it's much more intrusive than the other approaches. From my last discussion with @dalg24, we want an approach that keeps changes to existing files small (so it would be easy enough to revert in case we run into major problems). The compromise I'm proposing is to limit the changes to includes. |
Signed-off-by: Daniel Arndt <arndtd@ornl.gov>
…t configure-time" This reverts commit c022176. Signed-off-by: Daniel Arndt <arndtd@ornl.gov>
Signed-off-by: Daniel Arndt <arndtd@ornl.gov>
cd21e5b to
6bad1d1
Compare
Signed-off-by: Daniel Arndt <arndtd@ornl.gov>
6bad1d1 to
297f29a
Compare
This reverts commit 297f29a.
Part of #8117 to get started. Out of all the Kokkos functionalities,
StdAlgorithmsseems to be easiest to provide a module for.This pull request adds a build with clang-19 (currently in jenkins stage-1 for convenience; I'm happy to discuss moving whereever) and just lists all symbols needed explicitly. Since we are testing some Impl functionalities in the test, we split the module into
kokkos.stdalgorithmsandkokkos.stdalgorithms_implwith the understanding that the latter shouldn't be used downstream.The alternative to listing all symbolds would be duplicating all header files, possibly generating while configuring. #8117 demonstrates that for the containers.