Skip to content
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

[SYCL] Deprecate parallel_for and single_task overloads #16145

Draft
wants to merge 34 commits into
base: sycl
Choose a base branch
from

Conversation

HPS-1
Copy link
Contributor

@HPS-1 HPS-1 commented Nov 21, 2024

As the title says, added deprecation messages for such overloads in the sycl_ext_oneapi_kernel_properties extension, suggesting users to use single_task/parallel_for overloads provided in the sycl_ext_oneapi_enqueue_functions extension instead. (As these overloads are to be removed later as mentioned in #14785) Also fixed affected test cases by adding the Wno-deprecated-declarations build flag to let them ignore the deprecation warnings.

Also one point to notice:

  1. The following overload in the handler class is actually implemented as three function overloads with range<dimensions>... implemented as range<1>..., range<2>... and range<3>... respectively.
   template <typename KernelName, int dimensions, typename PropertyList, typename... Rest>
   void parallel_for(range<dimensions> numWorkItems,
                    PropertyList properties,
                    Rest&&... rest);

…xt_oneapi_kernel_properties extension

Signed-off-by: Hu, Peisen <[email protected]>
@HPS-1 HPS-1 requested a review from a team as a code owner November 21, 2024 06:19
@HPS-1 HPS-1 requested a review from againull November 21, 2024 06:19
@HPS-1 HPS-1 marked this pull request as draft November 21, 2024 06:21
@HPS-1 HPS-1 marked this pull request as ready for review November 22, 2024 04:40
@HPS-1 HPS-1 requested a review from a team as a code owner November 22, 2024 04:40
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

Graph tests LGTM

sycl/include/sycl/handler.hpp Outdated Show resolved Hide resolved
sycl/test-e2e/Basic/max_linear_work_group_size_props.cpp Outdated Show resolved Hide resolved
sycl/include/sycl/handler.hpp Outdated Show resolved Hide resolved
@HPS-1 HPS-1 changed the title [SYCL] Deprecate parallel_for and single_task overloads in the sycl_ext_oneapi_kernel_properties extension [SYCL] Deprecate parallel_for and single_task overloads Nov 26, 2024
Signed-off-by: Hu, Peisen <[email protected]>
@HPS-1 HPS-1 requested a review from a team as a code owner January 23, 2025 07:20
Signed-off-by: Hu, Peisen <[email protected]>
Signed-off-by: Hu, Peisen <[email protected]>
Signed-off-by: Hu, Peisen <[email protected]>
Signed-off-by: Hu, Peisen <[email protected]>
Copy link
Contributor

@againull againull left a comment

Choose a reason for hiding this comment

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

All deprecation messages indicate that sycl_ext_oneapi_enqueue_functions should be used instead of deprecated APIs.

But do I get it right that user also has a choice to use just regular SYCL 2020 parallel_for/single_task/parallel_for_work_group (queue and handler methods) but with kernel function object (which has a method to return properties) instead of a lambda? I.e. it seems like user doesn't have to use sycl_ext_oneapi_enqueue_functions. Maybe I am missing something.

Probably deprecation message should be fixed to indicate both available options.

sycl/include/sycl/handler.hpp Show resolved Hide resolved
sycl/include/sycl/handler.hpp Show resolved Hide resolved
sycl/include/sycl/queue.hpp Show resolved Hide resolved
@HPS-1 HPS-1 marked this pull request as draft January 23, 2025 22:18
Signed-off-by: Hu, Peisen <[email protected]>
@HPS-1
Copy link
Contributor Author

HPS-1 commented Jan 23, 2025

All deprecation messages indicate that sycl_ext_oneapi_enqueue_functions should be used instead of deprecated APIs.

But do I get it right that user also has a choice to use just regular SYCL 2020 parallel_for/single_task/parallel_for_work_group (queue and handler methods) but with kernel function object (which has a method to return properties) instead of a lambda? I.e. it seems like user doesn't have to use sycl_ext_oneapi_enqueue_functions. Maybe I am missing something.

Probably deprecation message should be fixed to indicate both available options.

Hi Artur, thanks for reviewing this! So just to let you know I've merged all commits in this PR into one in another PR #16728 to make the commit history cleaner. You can go there for further comments and check new updates.

And yes I agree with you -- the main point here is that if the user want to specify (compile-time) properties for a kernel, they can no longer specify such properties as arguments passed to (overloads of) single_task() and parallel_for(). Instead, they need to use kernel functors with the get(sycl::ext::oneapi::experimental::properties_tag) method to return properties. In other words, whether the single_task()/parallel_for() used are from original SYCL or extensions doesn't matter much (as long as they aren't among these deprecated ones). Sorry about the mis-wording here, I'll fix it in that other PR.

@againull
Copy link
Contributor

All deprecation messages indicate that sycl_ext_oneapi_enqueue_functions should be used instead of deprecated APIs.
But do I get it right that user also has a choice to use just regular SYCL 2020 parallel_for/single_task/parallel_for_work_group (queue and handler methods) but with kernel function object (which has a method to return properties) instead of a lambda? I.e. it seems like user doesn't have to use sycl_ext_oneapi_enqueue_functions. Maybe I am missing something.
Probably deprecation message should be fixed to indicate both available options.

Hi Artur, thanks for reviewing this! So just to let you know I've merged all commits in this PR into one in another PR #16728 to make the commit history cleaner. You can go there for further comments and check new updates.

And yes I agree with you -- the main point here is that if the user want to specify (compile-time) properties for a kernel, they can no longer specify such properties as arguments passed to (overloads of) single_task() and parallel_for(). Instead, they need to use kernel functors with the get(sycl::ext::oneapi::experimental::properties_tag) method to return properties. In other words, whether the single_task()/parallel_for() used are from original SYCL or extensions doesn't matter much (as long as they aren't among these deprecated ones). Sorry about the mis-wording here, I'll fix it in that other PR.

Sounds good, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants