[SYCL] Move Kernel specific data from `handler_impl` to a separate data structure #19843

vinser52 · 2025-08-20T20:50:44Z

This PR is a prerequisite for the handler-less API.
Kernel-specific data and argument parsing logic are moved from the handler_impl to the new KernelData class that will be used in a handler-less path.

vinser52 · 2025-08-20T21:01:45Z

sycl/source/detail/kernel_data.cpp

+      auto *DynParamImpl = static_cast<
+          ext::oneapi::experimental::detail::dynamic_parameter_impl *>(Ptr);
+
+      MDynamicParameters.emplace_back(DynParamImpl, Index + IndexShift);


There are two functional changes in this PR (this line and line 241). Before these changes, the handler::registerDynamicParameter method was used to add a dynamic parameter. The handler::registerDynamicParameter method has two checks that might throw exceptions. Since the processArg function is moved from handler to the KernelData we cannot use handler::registerDynamicParameter. I also cannot move the registerDynamicParameter to the KernelData class because it requires to access Queue and Graph from the handler.

aelovikov-intel

I was thinking about something like this: https://godbolt.org/z/jjG5zsb5z

@vinser52 , @sergey-semenov , WDYT?

The link above introduces

struct KernelData { /* information about device kernel */ };
struct SubmissionInfo { /* information about what's being submitted, including KernelData */ };

I think there will be a need for at least one other data structure to pass from handler to scheduler that will be encapsulating SubmissionInfo along with some other information filled in handler/no-handler submission path before reaching either scheduler or direct UR enqueue APIs.

sycl/source/detail/kernel_data.hpp

vinser52 · 2025-08-21T11:58:54Z

I was thinking about something like this: https://godbolt.org/z/jjG5zsb5z

@vinser52 , @sergey-semenov , WDYT?

The link above introduces
struct KernelData { /* information about device kernel */ };
struct SubmissionInfo { /* information about what's being submitted, including KernelData */ };
I think there will be a need for at least one other data structure to pass from handler to scheduler that will be encapsulating SubmissionInfo along with some other information filled in handler/no-handler submission path before reaching either scheduler or direct UR enqueue APIs.

In general, I have something similar in mind. We have compile time data that is captured in header and passed to the libsycl.so. And RT data, that could be split into two parts:

Data that does not change between kernel invokations/submissions. Today we are storing this data in the KernelNameBasedCache
Data that are specific to each particular invocation, like MArgs (we need to parse them on every submission).

My idea was that KernelData, which I introduced, would store all kernel-related data

sycl/include/sycl/handler.hpp

vinser52 · 2025-09-15T10:33:27Z

sycl/source/detail/device_kernel_info.cpp

    const CompileTimeKernelInfoTy &Info) {
  if (!isCompileTimeInfoSet())
    CompileTimeKernelInfoTy::operator=(Info);
-  assert(isCompileTimeInfoSet());


@sergey-semenov I removed this assert because in case of app was compiled with the 6.3 compiler, the kernel size is not passed from the headers to the runtime.

After discussion with @sergey-semenov, we decided to keep this assert in the preview mode.
Corresponding comment is added.

This deserves more explanation (probably in the PR description). New KernelData is under source/detail and doesn't cross ABI boundary. What are we changing here that has ABI effects and why can't that be done in a separate PR?

But I added comment in place. And the issue happens only in case of 6.3 or 6.2 headers. There is nothing about ABI here.

…in case of preview mode

reble

Graph part LGTM

github-actions · 2025-09-15T15:05:51Z

@intel/llvm-gatekeepers please consider merging

aelovikov-intel · 2025-09-15T15:22:44Z

Converting to draft to prevent a merge. I'll take a look at this later today.

vinser52 · 2025-09-15T15:39:16Z

Also, a review is needed from @sergey-semenov

aelovikov-intel · 2025-09-15T18:49:28Z

sycl/source/detail/kernel_data.hpp

+  DynamicParametersVecT MDynamicParameters;
+
+  /// The list of arguments for the kernel.
+  std::vector<detail::ArgDesc> MArgs;


That's what we do currently, but I think the goal should look more like

using HostKernelObjPtr = void *; using ExplicitlySetArgs = <whatever>; std::variant<HostKernelObjPtr, ExplicitlySetArgs> KernelParamsSource;

we can do it as a follow-up PR's. The goal of this PR just to split handler_impl and move some current logic/data to the KernelData and use KernelData in the no-handler flow.

aelovikov-intel · 2025-09-16T04:25:19Z

sycl/source/detail/handler_impl.hpp

+  // TODO: remove in the next ABI-breaking window
+  // Today they are used only in the handler::setKernelNameBasedCachePtr


This makes little sense to me. Everything under source/detail/ is inside the libsycl.so and doesn't cross ABI boundary.

These vars are used only when app is compiled with 6.3 compiler. In case of 6.3 compiler StoreLambda function calls setKerneInfo() that accept the compiler info from the integration header. After that StoreLambda calls setDeviceKernelInfoPtr. The problem with compatibility with 6.3 is that we need to store the data from integration header somewhere because DeviceKernelInfoPtr is not set yet.

You are right that this members does not cross ABI boundaries, but they are used only by the API that is under #ifndef __INTEL_PREVIEW_BREAKING_CHANGES macro. So I put them under preview, so that we don’t forget to remove them together with corresponding API in the next ABI-breaking window.

aelovikov-intel · 2025-09-16T04:28:51Z

sycl/source/detail/device_kernel_info.cpp

    const CompileTimeKernelInfoTy &Info) {
  if (!isCompileTimeInfoSet())
    CompileTimeKernelInfoTy::operator=(Info);
-  assert(isCompileTimeInfoSet());


This deserves more explanation (probably in the PR description). New KernelData is under source/detail and doesn't cross ABI boundary. What are we changing here that has ABI effects and why can't that be done in a separate PR?

aelovikov-intel · 2025-09-16T04:34:42Z

sycl/source/detail/kernel_data.cpp

+#ifndef __INTEL_PREVIEW_BREAKING_CHANGES
+                            ,
+                            bool IsESIMD
+#endif


Can we update the caller to do

KerneDataObject.IsESIMD = IsESIMD; KernelDataObject.proceesArg(/* no IsESIMD parameter*/);

instead? Or, in other words, do what we've done with the DeviceKernelInfoTy and have extra setting for the ABI compat entry points immediately after crossing the ABI boundary.

Unfortunately, we cannot because KernelData is not a flat data structure. The IsESIMD is stored in the DeviceKernelInfo and KernelData stores a pointer to it. In case of application is compiled with 6.2 headers the application calls processArg before the DeviceKernelInfo is assigned and we cannot create the device info at runtime because the kernel name is not assigned yet.

I disagree. That should be hacked on the handler side (where the problem is) and not here.

ok, let me check how it could be done on the handler side. I will try to do that.

sergey-semenov

LGTM. As agreed, I'm going to merge this to unblock PRs waiting for this one, but, naturally, feel free to continue the review after the merge.

…ta structure (intel#19843) This PR is a prerequisite for the handler-less API. Kernel-specific data and argument parsing logic are moved from the `handler_impl` to the new `KernelData` class that will be used in a handler-less path.

… updated in PR intel#19843. Doing so now. Without it there are CTS compat test issues Signed-off-by: Chris Perkins <chris.perkins@intel.com>

one of the overloads of extractArgsAndReqsFromLambda was to have been updated in PR #19843. Doing so now. Without it there are CTS compat test issues Signed-off-by: Chris Perkins <chris.perkins@intel.com>

vinser52 had a problem deploying to WindowsCILock August 20, 2025 20:50 — with GitHub Actions Error

vinser52 requested review from aelovikov-intel, sergey-semenov and slawekptak August 20, 2025 20:51

vinser52 commented Aug 20, 2025

View reviewed changes

vinser52 temporarily deployed to WindowsCILock August 20, 2025 21:09 — with GitHub Actions Inactive

vinser52 changed the title ~~[SYCL] Handler refactoring~~ [SYCL] Move Kernel specific data from handler_impl to a separate data structure Aug 20, 2025

vinser52 temporarily deployed to WindowsCILock August 20, 2025 21:34 — with GitHub Actions Inactive

vinser52 temporarily deployed to WindowsCILock August 20, 2025 23:02 — with GitHub Actions Inactive

aelovikov-intel reviewed Aug 20, 2025

View reviewed changes

aelovikov-intel reviewed Aug 21, 2025

View reviewed changes

sycl/source/detail/kernel_data.hpp Outdated Show resolved Hide resolved

vinser52 temporarily deployed to WindowsCILock August 21, 2025 15:10 — with GitHub Actions Inactive

vinser52 temporarily deployed to WindowsCILock August 21, 2025 15:36 — with GitHub Actions Inactive

vinser52 had a problem deploying to WindowsCILock August 21, 2025 15:36 — with GitHub Actions Failure

vinser52 temporarily deployed to WindowsCILock August 21, 2025 15:57 — with GitHub Actions Inactive

vinser52 force-pushed the handler_refactoring branch from d31cf35 to b479d9b Compare August 21, 2025 16:02

vinser52 temporarily deployed to WindowsCILock August 21, 2025 16:02 — with GitHub Actions Inactive

vinser52 temporarily deployed to WindowsCILock August 21, 2025 16:30 — with GitHub Actions Inactive

vinser52 force-pushed the handler_refactoring branch from b479d9b to 4c3b8fa Compare August 22, 2025 10:40

vinser52 temporarily deployed to WindowsCILock August 22, 2025 10:40 — with GitHub Actions Inactive

vinser52 temporarily deployed to WindowsCILock August 22, 2025 11:06 — with GitHub Actions Inactive

vinser52 temporarily deployed to WindowsCILock August 22, 2025 14:45 — with GitHub Actions Inactive

vinser52 mentioned this pull request Aug 25, 2025

[SYCL] Refactor kernel name based data approach #19117

Merged

vinser52 force-pushed the handler_refactoring branch from 4c3b8fa to a1c85b2 Compare August 28, 2025 15:34

vinser52 had a problem deploying to WindowsCILock August 28, 2025 15:34 — with GitHub Actions Failure

vinser52 had a problem deploying to WindowsCILock August 28, 2025 16:46 — with GitHub Actions Failure

vinser52 requested review from aelovikov-intel and slawekptak September 14, 2025 20:22

vinser52 temporarily deployed to WindowsCILock September 14, 2025 20:27 — with GitHub Actions Inactive

slawekptak reviewed Sep 15, 2025

View reviewed changes

sycl/include/sycl/handler.hpp Outdated Show resolved Hide resolved

sycl/include/sycl/handler.hpp Outdated Show resolved Hide resolved

Rename setKernelInfo to setKernelFunc

55d8ba2

vinser52 temporarily deployed to WindowsCILock September 15, 2025 09:48 — with GitHub Actions Inactive

slawekptak approved these changes Sep 15, 2025

View reviewed changes

vinser52 temporarily deployed to WindowsCILock September 15, 2025 10:32 — with GitHub Actions Inactive

vinser52 commented Sep 15, 2025

View reviewed changes

Returning assert to the DeviceKernelInfo::setCompileTimeInfoIfNeeded …

9a32a2f

…in case of preview mode

vinser52 temporarily deployed to WindowsCILock September 15, 2025 11:17 — with GitHub Actions Inactive

vinser52 temporarily deployed to WindowsCILock September 15, 2025 11:45 — with GitHub Actions Inactive

reble approved these changes Sep 15, 2025

View reviewed changes

aelovikov-intel marked this pull request as draft September 15, 2025 15:22

aelovikov-intel reviewed Sep 15, 2025

View reviewed changes

aelovikov-intel reviewed Sep 16, 2025

View reviewed changes

sergey-semenov approved these changes Sep 16, 2025

View reviewed changes

sergey-semenov marked this pull request as ready for review September 16, 2025 12:08

sergey-semenov merged commit c8ec91f into intel:sycl Sep 16, 2025
28 checks passed

cperkinsintel mentioned this pull request Sep 24, 2025

[SYCL] Refactor handler overload to fix CTS failure #20182

Merged

vinser52 mentioned this pull request Oct 7, 2025

[SYCL][NFC] Add a new header with kernel argument extraction logic #19831

Closed

		// TODO: remove in the next ABI-breaking window
		// Today they are used only in the handler::setKernelNameBasedCachePtr

[SYCL] Move Kernel specific data from handler_impl to a separate data structure #19843

[SYCL] Move Kernel specific data from handler_impl to a separate data structure #19843

Uh oh!

Conversation

vinser52 commented Aug 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vinser52 commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reble left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 15, 2025

Uh oh!

aelovikov-intel commented Sep 15, 2025

Uh oh!

vinser52 commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergey-semenov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[SYCL] Move Kernel specific data from `handler_impl` to a separate data structure #19843

[SYCL] Move Kernel specific data from `handler_impl` to a separate data structure #19843

aelovikov-intel left a comment •

edited

Loading

vinser52 commented Aug 21, 2025 •

edited

Loading