-
Notifications
You must be signed in to change notification settings - Fork 117
Move only type support for [transform_]reduce for host backends #2355
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
Conversation
a8d5077
to
d6d3860
Compare
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
5579d0f
to
9da7b42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for move-only types (types with only move constructor and move assignment operator) to the reduce
and transform_reduce
algorithms for host backends (TBB and OpenMP). The implementation ensures proper move semantics are used throughout the algorithm chains to avoid copy operations.
- Introduces
MoveOnlyWrapper
test utility class that enforces move-only semantics - Updates algorithm implementations to use
std::move()
for init values and intermediate results - Adds host-policy specific tests for move-only types in
reduce
andtransform_reduce
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
test/support/utils.h | Adds MoveOnlyWrapper template class for testing move-only type semantics |
test/parallel_api/numeric/numeric.ops/transform_reduce.pass.cpp | Updates test functions to support move semantics and adds move-only type tests |
test/parallel_api/numeric/numeric.ops/reduce.pass.cpp | Adds move-only type tests for reduce algorithm |
include/oneapi/dpl/pstl/utils.h | Updates comment formatting for __lazy_ctor_storage destructor |
include/oneapi/dpl/pstl/parallel_backend_tbb.h | Refactors TBB backend to use __lazy_ctor_storage and proper move semantics |
include/oneapi/dpl/pstl/parallel_backend_serial.h | Updates serial backend to use move semantics |
include/oneapi/dpl/pstl/omp/parallel_transform_reduce.h | Updates OpenMP backend to use move semantics throughout |
include/oneapi/dpl/pstl/numeric_impl.h | Updates brick functions and patterns to use move semantics |
include/oneapi/dpl/pstl/glue_numeric_impl.h | Updates public API functions to use move semantics |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Let me ask some overall questions about this change:
template <class _ExecutionPolicy, class _ForwardIterator, class _Tp, class _BinaryOperation, class... _Events,
oneapi::dpl::__internal::__enable_if_device_execution_policy_double_no_default<_ExecutionPolicy, int, _Tp,
_BinaryOperation, _Events...>>
auto
reduce_async(_ExecutionPolicy&& __exec, _ForwardIterator __first, _ForwardIterator __last, _Tp __init,
_BinaryOperation __binary_op, _Events&&... __dependencies)
{
const auto __dispatch_tag = oneapi::dpl::__internal::__select_backend(__exec, __first);
wait_for_all(std::forward<_Events>(__dependencies)...);
auto ret_val = oneapi::dpl::__internal::__pattern_transform_reduce_async(
__dispatch_tag, std::forward<_ExecutionPolicy>(__exec), __first, __last, __init, __binary_op,
oneapi::dpl::identity{});
return ret_val;
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor questions from me.
MoveOnlyWrapper(MoveOnlyWrapper&&) = default; | ||
|
||
// Move assignment operator | ||
MoveOnlyWrapper& operator=(MoveOnlyWrapper&&) = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a documentation update of our decision to require move assignment somewhere with the justification of why this is required to implement parallel reduce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can add to the documentation in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've augmented the documentation. I did not go into detail about why this is necessary as I do not really see a good location to have this discussion. It is not specific to parallel reduce, it even affects sequential reduce implementations. There is a way to implement it with only move construction, but it requires recursive handling which creates a stack overflow if the size is too large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akukanov, Would it be reasonable to link to https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0571r2.html from our documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the link for now.
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
@SergeyKopienko Sorry I missed this feedback.
What you refer to in the example is from the experimental async api. We can expand this change there, but I did not initially target this. Our testing should ensure that (at least for our testing coverage)
Using forwarding references is kind of strange here. We know exactly that this type is coming in as value to the API, and that we must always move it rather than copy it. I guess its another possible approach, but I'm not sure what we gain from that really. |
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR adds support within [transform_]reduce for types with only move constructor and move assignment operator (no copy assignment or construction) for host backends (tbb and omp).
It adds host policy tests for move only types within [transform_]reduce.pass.
This is part of the resolution for #1955.