Skip to content

Conversation

@HPS-1
Copy link
Contributor

@HPS-1 HPS-1 commented Oct 28, 2025

Basically to fix this issue. Note: seems that other than certain variants (overloads) of nd_launch which is explicitly mentioned in the issue, some variants of parallel_for with reductions are also having the same problem. Furthermore, these problematic nd_launch variants actually calls the problematic paralle_for variants, so in the end only parallel_for variants are modified.

Also there're some test revisions in this PR. These changes are previously blocked by the aforementioned issue. And as of the revision itself, the key idea here is to get rid of all parallel_for/single_task overloads that take a property list as a parameter (since these overloads are to be deprecated).

HPS-1 added 3 commits October 28, 2025 06:40
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
@HPS-1
Copy link
Contributor Author

HPS-1 commented Oct 28, 2025

@slawekptak Just to let you know that my work in this PR might address part of the problem you mentioned in https://github.com/intel/llvm/blame/40a9999b5536ac739e5f850c05d6d9dc1aa6e58b/sycl/include/sycl/ext/oneapi/experimental/enqueue_functions.hpp#L274, and thus pinging you to avoid repetitive work. Also feel free to correct me if I got anything wrong here. TY!

@slawekptak
Copy link
Contributor

@slawekptak Just to let you know that my work in this PR might address part of the problem you mentioned in https://github.com/intel/llvm/blame/40a9999b5536ac739e5f850c05d6d9dc1aa6e58b/sycl/include/sycl/ext/oneapi/experimental/enqueue_functions.hpp#L274, and thus pinging you to avoid repetitive work. Also feel free to correct me if I got anything wrong here. TY!

@HPS-1 Thanks for letting me know. I'm adding @uditagarwal97 who works on properties support for the handler-less path. This patch might not be directly related, as this addresses the handler path, but it is good to be aware of this change.

@HPS-1
Copy link
Contributor Author

HPS-1 commented Oct 30, 2025

@slawekptak Just to let you know that my work in this PR might address part of the problem you mentioned in https://github.com/intel/llvm/blame/40a9999b5536ac739e5f850c05d6d9dc1aa6e58b/sycl/include/sycl/ext/oneapi/experimental/enqueue_functions.hpp#L274, and thus pinging you to avoid repetitive work. Also feel free to correct me if I got anything wrong here. TY!

@HPS-1 Thanks for letting me know. I'm adding @uditagarwal97 who works on properties support for the handler-less path. This patch might not be directly related, as this addresses the handler path, but it is good to be aware of this change.

Thanks! And just a mention that I believe this is actually related to the handler-less path since the handler-less path implicitly calls the handler path sometimes: as you can see in the nd_launch implementation for the handler-less path:

template <typename KernelName = sycl::detail::auto_name, int Dimensions,
          typename KernelType, typename... ReductionsT>
void nd_launch(queue Q, nd_range<Dimensions> Range, const KernelType &KernelObj,
               ReductionsT &&...Reductions) {
  // TODO The handler-less path does not support reductions, kernel
  // function properties and kernel functions with the kernel_handler
  // type argument yet.
  if constexpr (sizeof...(ReductionsT) == 0 &&
                !(ext::oneapi::experimental::detail::
                      HasKernelPropertiesGetMethod<
                          const KernelType &>::value) &&
                !(detail::KernelLambdaHasKernelHandlerArgT<
                    KernelType, sycl::nd_item<Dimensions>>::value)) {
    detail::submit_kernel_direct_parallel_for<KernelName>(
        std::move(Q), empty_properties_t{}, Range, KernelObj);
  } else {
    submit(std::move(Q), [&](handler &CGH) {
      nd_launch<KernelName>(CGH, Range, KernelObj,
                            std::forward<ReductionsT>(Reductions)...);
    });
  }

Here in the if statement, it actually calls the nd_launch variant for the handler path when the kernel functor uses a get(properties_tag) to specifies its properties (i.e. when ext::oneapi::experimental::detail::HasKernelPropertiesGetMethod<const KernelType &>::value evaluates to true).

Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
@HPS-1 HPS-1 marked this pull request as ready for review November 10, 2025 01:37
@HPS-1 HPS-1 requested a review from a team as a code owner November 10, 2025 01:37
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
@HPS-1
Copy link
Contributor Author

HPS-1 commented Nov 21, 2025

Hi @aelovikov-intel & @uditagarwal97 ! I believe this PR is good to go now, could you please take a look and approve it if applicable? This way I can merge it and close the respective issue. Thank you in advance!

Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

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

lgtm

@HPS-1
Copy link
Contributor Author

HPS-1 commented Nov 25, 2025

@intel/llvm-gatekeepers Hi could you please merge this? Thanks! (The test failure is irrelevant)

@uditagarwal97 uditagarwal97 merged commit d1478de into intel:sycl Nov 26, 2025
60 of 66 checks passed
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.

Can't Specify Properties in sycl_ext_oneapi_kernel_properties with Reductions

4 participants