-
Notifications
You must be signed in to change notification settings - Fork 116
Remove make_iter_mode and refactor access mode resolution
#2551
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
base: dev/dhoeflin/unitialized_refactors
Are you sure you want to change the base?
Remove make_iter_mode and refactor access mode resolution
#2551
Conversation
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 centralizes access mode resolution logic for SYCL iterators by moving it from scattered make_iter_mode calls to the leaf-level __get_sycl_range implementation. The refactoring removes the iter_mode struct and make_iter_mode function, replacing them with compile-time mode resolution directly in __process_input_iter for sycl_iterator types.
Key Changes:
- Removed
make_iter_modepattern and moved access mode resolution into__get_sycl_range'ssycl_iteratorspecialization - Fixed incorrect mode compatibility logic (e.g., read/write iterators can no longer satisfy read_write algorithm requirements)
- Added comprehensive test coverage for mode resolution compatibility checking
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
parallel_backend_sycl.h |
Removed deprecated iter_mode_resolver, iter_mode struct, and make_iter_mode function (~130 lines) |
utils_ranges_sycl.h |
Added refactored iter_mode_resolver with corrected compatibility logic and new sycl_iterator specialization in __process_input_iter |
algorithm_impl_hetero.h |
Removed make_iter_mode wrapper calls from fill, generate, partition_copy, inplace_merge, and partial_sort patterns |
async_impl_hetero.h |
Removed make_iter_mode wrapper calls from async fill pattern |
get_sycl_range.pass.cpp |
Added test_is_iter_mode_resolvable_v test function covering valid/invalid mode combinations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e4040d0 to
5bf81d6
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
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
36ae926 to
03c83d1
Compare
044c431 to
728d20c
Compare
03c83d1 to
08eb860
Compare
808f0de to
be5fe30
Compare
|
At first glance, I am in favor of the changes. I didn’t like Thank you! |
| //try searching for the first element which not equal to *__b | ||
| if (__b != __first1) | ||
| __b += __internal::__pstl_upper_bound(__b, _DifferenceType1{0}, __last1 - __b, __b, __comp, __proj1, __proj1); | ||
| __b += __internal::__pstl_upper_bound(__b, _DifferenceType1{0}, __last1 - __b, __b, __comp, __proj1, |
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.
Let's save the previous format.
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.
removed these formatting only changes. thanks.
| //try searching for the first element which not equal to *__e | ||
| if (__e != __last1) | ||
| __e += __internal::__pstl_upper_bound(__e, _DifferenceType1{0}, __last1 - __e, __e, __comp, __proj1, __proj1); | ||
| __e += __internal::__pstl_upper_bound(__e, _DifferenceType1{0}, __last1 - __e, __e, __comp, __proj1, |
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.
Let's save the previous format.
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.
removed these formatting only changes. thanks.
08eb860 to
f02042d
Compare
fix access modes for uninitialized apis remove unnecessary pattern Signed-off-by: Dan Hoeflinger <[email protected]>
fix access modes for uninitialized apis remove unnecessary pattern 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]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
be5fe30 to
c0563c6
Compare
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Summary
Move access mode resolution logic from scattered
make_iter_modecalls to the leaf-level__get_sycl_rangewhen processingsycl_iterator. This centralizes the mode resolution logic and ensures it properly recurses through nested iterator types.Changes
Removed
make_iter_modepattern:iter_modestruct andmake_iter_modefunction fromparallel_backend_sycl.hmake_iter_modewrapper calls from algorithm implementations inalgorithm_impl_hetero.handasync_impl_hetero.hRefactored
iter_mode_resolver:utils_ranges_sycl.has the authoritative location__is_iter_mode_resolvable_vtrait for testable compile-time compatibility checkingnoInitto track whether algorithm allows skipping host→device copyFixed mode compatibility logic:
readorwriteiterators could satisfyread_writealgorithm requirementsdiscard_*modes with algorithms that dont have no_initread_write→read,read_write→write,discard_read_write->*(only whenno_init=true),discard_write->write(only whenno_init=true).Connected no_init property to accessors:
all_viewnow includes a template argument for_NoInitwhich connects to actual sycl accessorno_initproperty when appropriateNOTE: These changes could result in new build error for users where it was not before if it was relying upon improperly matched behavior. It seems like this could've resulted in silent problems or even segfaults in kernels where algorithms don't get the access modes they need.
Added tests:
test_is_iter_mode_resolvable_v()covering valid and invalid mode combinationsTargeted to land after #2519 and #2549 (final episode in the
__get_sycl_rangerefactor trilogy)Resolves Issue #2550
Future work:
There may be some opportunity to utilize
no_initfor our temporary buffers, though I'm not sure if that will result in any real savings, as the system should know when there is nothing to transfer to the device.