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

Open
wants to merge 7 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 two points 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);
  1. The following overloads in the queue class appear to have not been implemented so deprecation warnings are not needed here:
  template <typename KernelName, int Dims, typename PropertyList, typename... Rest>
  event parallel_for(range<Dims> numWorkItems,
                     Rest&&... rest);

  template <typename KernelName, int Dims, typename PropertyList, typename... Rest>
  event parallel_for(range<Dims> numWorkItems, event depEvent,
                     PropertyList properties,
                     Rest&&... rest);

  template <typename KernelName, int Dims, typename PropertyList, typename... Rest>
  event parallel_for(range<Dims> numWorkItems,
                     const std::vector<event> &depEvents,
                     PropertyList properties,
                     Rest&&... rest);

  template <typename KernelName, int Dims, typename PropertyList, typename... Rest>
  event parallel_for(nd_range<Dims> executionRange,
                     event depEvent,
                     PropertyList properties,
                     Rest&&... rest);

  template <typename KernelName, int Dims, typename PropertyList, typename... Rest>
  event parallel_for(nd_range<Dims> executionRange,
                     const std::vector<event> &depEvents,
                     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

std::enable_if_t<
ext::oneapi::experimental::is_property_list<PropertiesT>::value>
single_task(PropertiesT Props, _KERNELFUNCPARAM(KernelFunc)) {
__SYCL2020_DEPRECATED(
Copy link
Contributor

Choose a reason for hiding this comment

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

SYCL2020 deprecated means that this feature is marked as deprecated in the core SYCL 2020 specification and it will be removed in whatever next version of the core SYCL specification there will be.

However, in this case we are deprecating an overload documented by an extension - it is not related to the SYCL language version and I assume that we want it to be removed way earlier than we drop SYCL 2020 support. Therefore, regular SYCL_DEPRECATED macro should be used

@@ -1,4 +1,4 @@
// RUN: %{build} -o %t.out
// RUN: %{build} -o %t.out -Wno-deprecated-declarations
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we specifically want to test the old API, we shouldn't add this flag but rewrite the test instead. The reason is fear that we can accidentally use even more deprecated APIs in a test, thus making even more work for ourselves in the future PR which removes deprecated overloads.

detail::AreAllButLastReductions<RestT...>::value &&
ext::oneapi::experimental::is_property_list<PropertiesT>::value>
parallel_for(range<1> Range, PropertiesT Properties, RestT &&...Rest) {
__SYCL2020_DEPRECATED(
Copy link
Contributor

Choose a reason for hiding this comment

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

A new test should be added into sycl/test/warnings/ to check that deprecated message is correct and emitted for all overloads

@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
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.

3 participants