Batched: allowing dynrankview in get_extent get_stride#3036
Batched: allowing dynrankview in get_extent get_stride#3036lucbv merged 8 commits intokokkos:developfrom
Conversation
Some downstream libraries and applications like Intrepid2 use dynamic rank views and we need to allow them to get passed in as input parameters. Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>
yasahi-hpc
left a comment
yasahi-hpc
left a comment
There was a problem hiding this comment.
We cannot make runtime checks in these helpers.
Rather, it is better to add runtime checks in implementation details of TeamGemm.
Since it is not implemented for rank 3+, it is safer to check these in non-debug mode as well
#ifndef NDEBUG
if constexpr (Kokkos::is_dyn_rank_view_v<AViewType >) {
if (A.rank() > 2) return 1;
}
if constexpr (Kokkos::is_dyn_rank_view_v<BViewType >) {
if (B.rank() > 2) return 1;
}
if constexpr (Kokkos::is_dyn_rank_view_v<CViewType >) {
if (C.rank() > 2) return 1;
}
#endif
Signed-off-by: Luc Berger <lberge@sandia.gov>
Signed-off-by: Luc Berger <lberge@sandia.gov>
Signed-off-by: Luc Berger <lberge@sandia.gov>
|
I removed the runtime check on the DynRankView but I keep the check on being a view or DynRankView |
Signed-off-by: Luc Berger <lberge@sandia.gov>
There was a problem hiding this comment.
I have changed my mind.
Although this check is inconsistent with other checks in batched APIs, we need to ensure that this helper is not used for rank 3+ view.
Still, it is preferable to do this kind of rank checks in the implementation detail.
You also need to include #include <Kokkos_Assert.hpp>
We really do require these views to have only rank 0, 1 or 2 as we later on index with the raw pointer directly and will only use two extents and strides to do so... Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>
Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>
Signed-off-by: Luc Berger-Vergiat <lberge@sandia.gov>
ndellingwood
left a comment
ndellingwood
left a comment
There was a problem hiding this comment.
@lucbv should this be cherry-picked to 5.1.1, or does this workaround changes after the 5.1.0 release?
|
The additional checks might break downstream users so it might be good to do it in 5.2.0 to have a full integration with Trilinos |
|
@lucbv should we test this with Trilinos@develop before merge? |
|
@ndellingwood we should at least test once against Trilinos in my opinion, this restricts a bit the API so it could bite users... |
Concerning that this previously does not accept DynRankView, I think this PR still relaxes the usage |
|
No failures in the nightly Trilinos jobs due to this change last night 👍 |
3 participants
Supersedes #3034 @japlews
Some downstream libraries and applications like Intrepid2 use dynamic rank views and we need to allow them to get passed in as input parameters.