Skip to content

[SYCL] Do not make auto& range parallel_for a future breaking change #11897

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 29, 2023
Merged

[SYCL] Do not make auto& range parallel_for a future breaking change #11897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bef9dd
[SYCL] Do not make auto& range parallel_for a future breaking change
jzc Nov 15, 2023
c6cafce
Merge remote-tracking branch 'intel/sycl' into auto-ref-no-breaking
jzc Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions sycl/include/sycl/handler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1261,9 +1261,10 @@ class __SYCL_EXPORT handler {
"first argument of sycl::item type, or of a type which is "
"implicitly convertible from sycl::item");

using RefLambdaArgType = std::add_lvalue_reference_t<LambdaArgType>;
static_assert(
(std::is_invocable_v<KernelType, LambdaArgType> ||
std::is_invocable_v<KernelType, LambdaArgType, kernel_handler>),
(std::is_invocable_v<KernelType, RefLambdaArgType> ||
std::is_invocable_v<KernelType, RefLambdaArgType, kernel_handler>),
"SYCL kernel lambda/functor has an unexpected signature, it should be "
"invocable with sycl::item and optionally sycl::kernel_handler");
#endif
Expand Down
5 changes: 0 additions & 5 deletions sycl/test/basic_tests/handler/handler_generic_lambda_interface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,6 @@ int main() {
test_parallel_for_work_group<class Group2Name, sycl::group<2>>(sycl::range<2>{1, 1});
test_parallel_for_work_group<class Group3Name, sycl::group<3>>(sycl::range<3>{1, 1, 1});

#ifndef PREVIEW_BREAKING_CHANGES
// For backwards compatibility, we still support some invalid variants of
// kernel lambdas. These test cases should be removed once we make conformant
// mode the default.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. The comment here says that we still support invalid variants and hides the tests behind the #ifndef .

But here you are removing the guards and just running the tests. So these variants are not invalid?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking, they are invalid according to the current wording of SYCL 2020 spec. However, we have received feedback from @gmlueck that in this case we would like to lift the spec restrictions instead of breaking existing code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually think this is a bug in the spec, and the spec should allow "auto &". I have a TODO task to propose a PR that changes the spec.

sycl::queue q;
q.submit([&](sycl::handler &cgh) {
cgh.parallel_for(sycl::range{1}, [=](auto &) {});
Expand All @@ -67,7 +63,6 @@ int main() {
q.submit([&](sycl::handler &cgh) {
cgh.parallel_for(sycl::range{1, 1, 1}, [=](auto &) {});
});
#endif

return 0;
}
12 changes: 0 additions & 12 deletions sycl/test/basic_tests/handler/parallel_for_arg_restrictions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,18 +71,6 @@ int main() {
CGH.parallel_for(sycl::nd_range{sycl::range{1, 1, 1}, sycl::range{1, 1, 1}},
[=](ConvertibleFromItem<3>) {});
});
Q.submit([&](sycl::handler &CGH) {
// expected-error@sycl/handler.hpp:* {{SYCL kernel lambda/functor has an unexpected signature, it should be invocable with sycl::item and optionally sycl::kernel_handler}}
CGH.parallel_for(sycl::range{1}, [=](auto &) {});
});
Q.submit([&](sycl::handler &CGH) {
// expected-error@sycl/handler.hpp:* {{SYCL kernel lambda/functor has an unexpected signature, it should be invocable with sycl::item and optionally sycl::kernel_handler}}
CGH.parallel_for(sycl::range{1, 1}, [=](auto &) {});
});
Q.submit([&](sycl::handler &CGH) {
// expected-error@sycl/handler.hpp:* {{SYCL kernel lambda/functor has an unexpected signature, it should be invocable with sycl::item and optionally sycl::kernel_handler}}
CGH.parallel_for(sycl::range{1, 1, 1}, [=](auto &) {});
});

Q.submit([&](sycl::handler &CGH) {
// expected-error@sycl/handler.hpp:* {{sycl::parallel_for(sycl::range) kernel must have the first argument of sycl::item type, or of a type which is implicitly convertible from sycl::item}}
Expand Down