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

Refactor: Replace SFINAE by if constexpr for create_mirror* functions #6955

Merged
merged 27 commits into from
May 23, 2024

Conversation

pzehner
Copy link
Contributor

@pzehner pzehner commented Apr 19, 2024

This PR is a refactor without side effects.

The idea is to simplify the code of the create_mirror* functions by applying techniques of the C++ 17 standard. A refactor with the similar approach was drafted by @masterleinad in #5533 about two years ago, but never merged.

The multiple overloaded functions of Impl::create_mirror, Impl::create_mirror_view, and Impl::create_mirror_view_and_copy that use SFINAE are replaced by one function that uses if constexpr instead (for Kokkos views, offset views, dynamic views and dynamic rank views). Eventually, the return type of these functions is replaced by auto, and decided depending on which branch of the if constexpr is used.

The logic of the public create_mirror* functions (which is sometimes redundant with the logic of the private functions) is not altered in this PR and will be assessed in an upcoming one.

This PR is part of a of a larger effort which aims to assess #6842.

We successfully built and tested the refactored code for the following configurations:

Compiler Version Min. ver. Backend Version Min. ver.
Clang 14.0.0 8.0.0 Serial
GCC 8.4.0 8.2.0 Serial
GCC 8.4.0 8.2.0 OpenMP
GCC 8.4.0 8.2.0 CUDA 11.4 11.0
Intel Classic 2023.2.1 19.0.5 OpenMP
Intel LLVM 2023.2.1 2021.1.1 OpenMP
Intel LLVM 2024.0.2 2022.0.0 SYCL
HIP 5.2.0 5.2.0 ROCM

where "Compiler" is the used compiler, next "Version" is the version of the compiler, next "Min. Vers." is the minimum version of the compiler for Kokkos 4.3.0 according to the documentation, "Backend" is the name of the backend, next "Version" is the version for the backend if necessary, and next "Min. Vers." is the minimum version of the backend supported.

Unfortunately, we could not test any earlier version of the compilers, nor of the backends, on our test machines. Also, please note that some configurations could not be used:

  • Clang with OpenMP or CUDA;
  • MSVC; and
  • NVHPC.

Finally, the SYCL configuration was built, but not tested.

containers/src/Kokkos_DynRankView.hpp Outdated Show resolved Hide resolved
containers/src/Kokkos_DynRankView.hpp Outdated Show resolved Hide resolved
containers/src/Kokkos_DynRankView.hpp Outdated Show resolved Hide resolved
containers/src/Kokkos_DynRankView.hpp Show resolved Hide resolved
@pzehner pzehner marked this pull request as draft April 22, 2024 14:07
@masterleinad
Copy link
Contributor

You'll have to fix indentation.

@pzehner pzehner marked this pull request as ready for review April 29, 2024 12:56
@pzehner
Copy link
Contributor Author

pzehner commented Apr 29, 2024

The code style was fixed using clang-format.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks OK to me but we should test with Trilinos/Sacado/Stokhos just to make sure this doesn't break their customizations.

@masterleinad
Copy link
Contributor

Can you please rebase on top of develop (or merge) to avoid running with broken configurations?

@pzehner
Copy link
Contributor Author

pzehner commented Apr 29, 2024

Merging done. For merging, can you squash the commits, as the history of this branch is quite messy?

@masterleinad
Copy link
Contributor

Merging done. For merging, can you squash the commits, as the history of this branch is quite messy?

Yes, we normally squash commits before merging.

@pzehner
Copy link
Contributor Author

pzehner commented Apr 30, 2024

I think I merged the latest version of develop, but the same build errors not related to the PR are still showing up.

@pzehner
Copy link
Contributor Author

pzehner commented Apr 30, 2024

we should test with Trilinos/Sacado/Stokhos just to make sure this doesn't break their customizations.

Could you recommend any build script for Trilinos, in order to test it?

@masterleinad
Copy link
Contributor

I think I merged the latest version of develop, but the same build errors not related to the PR are still showing up.

Yes, it seems an update in our CI containers is responsible for the build failures. #6969 fixes one of them.

@crtrott
Copy link
Member

crtrott commented May 20, 2024

Retest this please!

@pzehner
Copy link
Contributor Author

pzehner commented May 23, 2024

@crtrott Could you validate this PR? It seems the failing jobs are due to external factors.

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

4 participants