Only fence in SYCL parallel_reduce for non-device-accessible result_ptr #4089
Conversation
masterleinad
force-pushed
the
sycl_reduce_fencing_reduce
branch
from
0523bca
to
92df7c4
Compare
| @@ -336,6 +356,7 @@ class ParallelReduce<FunctorType, Kokkos::RangePolicy<Traits...>, ReducerType, | |||
| Policy m_policy; | |||
| ReducerType m_reducer; | |||
| pointer_type m_result_ptr; | |||
| const bool m_result_ptr_device_accessible; | |||
There was a problem hiding this comment.
Minor: If we are going to make this const, shouldn't we make the other member vars const as well?
There was a problem hiding this comment.
What is he point of making any of them const?
#4088 (comment)
There was a problem hiding this comment.
I could go either way here. It doesn't seem sensible to store a copy of a parallel_reduce/parallel_for/parallel_scan object as a class member or invoke the copy-assignment operator in a different way. Having it const makes sure that we don't forget to initialize it (and express intent: none of the members is supposed to change after calling the constructor).
That being said, we are not consistent across the SYCL implementations of these functions. If you feel strongly about it, I would rather try to make these classes copy-assignable in a separate pull request.
Based on top of #3940. Only 0523bca is relevant.
Before this, we also fenced for device-accessible result_ptr.