Add test for TeamPolicy array reduction #6296
Conversation
masterleinad
force-pushed
the
test_team_policy_array_reduction
branch
from
2f5cdc9
to
b4965c5
Compare
masterleinad
force-pushed
the
test_team_policy_array_reduction
branch
from
731abb1
to
ea26947
Compare
masterleinad
force-pushed
the
test_team_policy_array_reduction
branch
from
1042731
to
ea26947
Compare
|
why is this PR also touching impl for CUDA and HIP too? rather than just being about a new test? |
Because the code path used for array reductions did not compile for CUDA and HIP. |
masterleinad
added
the
Bug
|
Retest this please. |
1 similar comment
|
Retest this please. |
core/unit_test/TestTeam.hpp
Outdated
| using value_type = ScalarType[]; | ||
| size_type value_count = 3; | ||
|
|
||
| const size_type nwork; |
There was a problem hiding this comment.
All this is copied from ReduceTeamFunctor and TeamFunctor (array reduction for RangePolicy).
core/unit_test/TestTeam.hpp
Outdated
| KOKKOS_INLINE_FUNCTION | ||
| ArrayReduceTeamFunctor(const ArrayReduceTeamFunctor &rhs) | ||
| : nwork(rhs.nwork) {} |
There was a problem hiding this comment.
Why define the copy constructor at all?
core/unit_test/TestTeam.hpp
Outdated
| dst[0] = 0; | ||
| dst[1] = 0; | ||
| dst[2] = 0; |
There was a problem hiding this comment.
for (int i = 0; i < value_count; ++i) ...
| kokkos_impl_cuda_shared_memory<word_size_type>() + | ||
| threadIdx.y * word_count.value); | ||
| reference_type value = | ||
| m_functor_reducer.get_reducer().init(reinterpret_cast<pointer_type>( |
There was a problem hiding this comment.
So it is not so much a "adding test", the code was broken wasn't it?
There was a problem hiding this comment.
Well, yeah, it was broken for Cuda and HIP but not for the other backends AFAIK which means we didn't have a test.
core/unit_test/TestTeam.hpp
Outdated
| } | ||
|
|
||
| KOKKOS_INLINE_FUNCTION | ||
| void operator()(const typename policy_type::member_type ind, |
There was a problem hiding this comment.
Not blocking but I am really not a fan of calling it "ind".
Also why are we not passing by ref?
core/unit_test/TestTeam.hpp
Outdated
| const uint64_t correct = 0 == j % 3 ? nw : nsum; | ||
| ASSERT_EQ((ScalarType)correct, result[i][j]); |
There was a problem hiding this comment.
This feels silly, just inline the assertion and inject more information with a << <helpful message>;
In reference to #6293 (comment). It seems that we don't test array reductions for TeamPolicy in the CI. This pull request copies a very similar test to use array reductions instead of a C-style array of fixed size.