Add static_assert check for std::range::ref_view in SYCL-kernel code
#2505
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds compile-time checks to detect accidental use of std::ranges::views like ref_view that contain host pointers in SYCL kernels. The changes introduce a new MinimalisticRange type and refactor the testing infrastructure to verify that oneDPL properly rejects ranges containing host pointers while allowing safe views.
- Introduces
__contains_host_pointertrait to detect ranges with host pointers at compile time - Refactors
MinimalisticViewto inherit from newMinimalisticRangebase type - Adds comprehensive static assertion tests for various view types (ref_view, zip_view, take_view, drop_view)
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/support/utils.h | Splits MinimalisticView into base MinimalisticRange and derived MinimalisticView to support different test scenarios |
| test/parallel_api/ranges/range_minimal_type_requirements.pass.cpp | Adds compile-time error generation macros and new test_count_if test case with PredRef helper |
| test/general/implementation_details/subscription_view.pass.cpp | Adds extensive static assertion tests for host pointer detection across various view compositions |
| include/oneapi/dpl/pstl/utils_ranges.h | Refactors pipeline_base_range to __pipeline_base_range with improved type extraction |
| include/oneapi/dpl/pstl/hetero/dpcpp/utils_ranges_sycl.h | Implements __contains_host_pointer detection mechanism and integrates static assertions into access requirement logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/parallel_api/ranges/range_minimal_type_requirements.pass.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…layer into struct pipeline_base
…pe / std::false_type underneath the structure struct pipeline_base_range
… nested view into struct pipeline_base_range
…struct __contains_host_pointer_on_any_layers and etc. staff
…e-time validation to check if the View is reference-based
…listicRange and struct MinimalisticView
…tional checks to ensure the new compile-time validation to checks (if the View is reference-based)
…ntroduce generation of compile-time errors (under switched off defines)
SergeyKopienko
force-pushed
the
dev/skopienko/detect_ref_view_on_compile_time
branch
from
2f6678b to
34368c5
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…oduce contains_host_pointer_on_any_layers_v for test purposes
…ra std::remove_cvref_t<_View> from struct __contains_host_pointer_on_any_layers
…range::get_range_on_next_layer() as unused in code
…itions from struct pipeline_base
…std::true_type / std::false_type) instead of inheritance
…n_next_layer -> struct pipeline_base_range::next_layer_view_t
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SergeyKopienko
force-pushed
the
dev/skopienko/detect_ref_view_on_compile_time
branch
from
9a7834c to
fea0d4d
Compare
… move new tests into separate file
…evert extra changes
test/general/implementation_details/contains_host_pointers.pass.cpp
Outdated
Show resolved
Hide resolved
… remove extra checks
| static_assert(!__contains_host_pointer_on_any_layers<_BaseRange>::value, | ||
| "oneDPL does not support std::ranges::ref_view in SYCL-kernel code"); |
There was a problem hiding this comment.
Will you add a corresponding comment into the release notes or oneDPL guide? I think it is worth mentioning.
There was a problem hiding this comment.
@timmiesmith, what is the best place to document this compile-time check?
There was a problem hiding this comment.
To be more precise, it's not about the check per se, but about the fact that that one should not pass anything for execution with device policies which can create std::ranges::ref_view underneath, e.g. std::ranges::views::all wrapping a range which is not a view.
My recommendation is the release notes for the upcoming release if we believe it is temporary and known limitations in the library guide if its for long. I am inclined towards the later.
There was a problem hiding this comment.
I think for now it is better considered a temporary limitation.
test/general/implementation_details/contains_host_pointers.pass.cpp
Outdated
Show resolved
Hide resolved
… remove extra comments
dmitriy-sobolev
left a comment
dmitriy-sobolev
left a comment
There was a problem hiding this comment.
Looks good as it is.
I'm ready to reapprove if the documentation note is added, but it can be done separately.
…_view specialization to accept variadic template parameters
…w, _Views...> and extra test functional has been remove
… comment: simplify __contains_host_pointer staff
|
No more questions for the implementation, but I have not reviewed the test. |
3 participants
This PR adds compile-time checks to detect accidental use of
std::ranges::ref_view: it contains raw-pointer inside with the address of referenced view. In the case of SYCL-kernel we still have here pointer with the address of host memory.The changes introduce a new
MinimalisticRangetype and refactor the testing infrastructure to verify that oneDPL properly rejects ranges containing host pointers while allowing safe views.__contains_host_pointertrait to detect ranges with host pointers at compile timeMinimalisticViewto inherit from newMinimalisticRangebase typeDetails
std::range::allAPI function can returnstd::ref_viewas described here link1, link2