Improve performance for SYCL parallel_reduce #3732
Conversation
masterleinad
force-pushed
the
sycl_reduce_performance
branch
2 times, most recently
from
7c46954
to
d238a47
Compare
masterleinad
force-pushed
the
sycl_reduce_performance
branch
from
d238a47
to
b9844de
Compare
masterleinad
changed the title
|
Benchmark showed better performance for the recursive implementation so I dropped the commit related to the one that lets the last block do the final reduction. A backup for that implementation can be found at https://github.com/masterleinad/kokkos/tree/sycl_reduce_performance_backup. |
masterleinad
force-pushed
the
sycl_reduce_performance
branch
from
b9844de
to
6fc5a11
Compare
masterleinad
force-pushed
the
sycl_reduce_performance
branch
from
bce6cc5
to
19eda78
Compare
masterleinad
force-pushed
the
sycl_reduce_performance
branch
2 times, most recently
from
19e1c92
to
1f32a62
Compare
| const typename Policy::index_type upper_bound = | ||
| (global_id + values_per_thread * wgroup_size) < size | ||
| ? global_id + values_per_thread * wgroup_size | ||
| : size; |
There was a problem hiding this comment.
std::min might be more readable than this ternary op
There was a problem hiding this comment.
Yes, I wasn't quite sure if that's supported on the device. I can check.
I thought I didn't change it too much but I'm happy to add some more comments. |
| const auto init_size = std::max<std::size_t>( | ||
| ((size + values_per_thread - 1) / values_per_thread + wgroup_size - 1) / | ||
| wgroup_size, | ||
| 1); | ||
| const auto value_count = | ||
| FunctorValueTraits<ReducerTypeFwd, WorkTagFwd>::value_count( | ||
| selected_reducer); | ||
| const auto results_ptr = | ||
| static_cast<pointer_type>(Experimental::SYCLSharedUSMSpace().allocate( | ||
| "SYCL parallel_reduce result storage", | ||
| sizeof(*m_result_ptr) * std::max(value_count, 1u) * init_size)); |
There was a problem hiding this comment.
Yes, sizeof(value_type) should be the same. Let me try that.
| const auto init_size = | ||
| std::max<std::size_t>((size + wgroup_size - 1) / wgroup_size, 1); | ||
| constexpr size_t wgroup_size = 128; | ||
| constexpr size_t values_per_thread = 2; |
There was a problem hiding this comment.
this values per thread, is probably right for something like dot product, but probably wrong for LAMMPS where the reduction is on rather beefy kernels which often don't have enough parallelism (think 30k atoms, each looping serially over its neighbors and then do a reduction over all of them) not sure if you care right now
- let the last block do the final reduction