[SYCL] enable_shared_from_this for kernel_bundle_impl #18899

Alexandr-Konovalov · 2025-06-10T15:32:30Z

This allow us to get rid of passing self to kernel_bundle_impl methods and to return raw pointer to kernel bundle instead of shared_ptr.

…undle_impl

aelovikov-intel · 2025-06-10T21:13:17Z

sycl/source/detail/kernel_bundle_impl.hpp

+    std::shared_ptr<kernel_bundle_impl> Self =
+        const_cast<kernel_bundle_impl *>(this)->shared_from_this();


Temporary until tryGetSourceBasedKernel is updated, right?

Sorry, I missed the idea. How is it related to tryGetSourceBasedKernel() and what is temporary?

aelovikov-intel · 2025-06-10T21:18:13Z

sycl/source/detail/kernel_bundle_impl.hpp

@@ -938,13 +947,15 @@ class kernel_bundle_impl {
            SelectedImage->get_ur_program_ref());

    return std::make_shared<kernel_impl>(
-        Kernel, detail::getSyclObjImpl(MContext), SelectedImage, Self, ArgMask,
+        Kernel, detail::getSyclObjImpl(MContext), SelectedImage,
+        const_cast<kernel_bundle_impl *>(this)->shared_from_this(), ArgMask,


I think we can do

class kernel_bundle_impl : public std::enable_shared_from_this<kernel_bundle_impl> { using Base = std::enable_shared_from_this<kernel_bundle_impl>; public std::shared_ptr<kernel_bundle_impl> shared_from_this() const { return const_cast<...>(this)->Base::shared_from_this(); } };

If compiler would complain about incomplete types, make it a template:

template <typename Self = kernel_bundle_impl> ... shared_from_this() const { Self *self = this; // use `self` instead of `this` to make template-type-dependent }

So the idea is to have ugly cast in single place, right? Done.

I'm actually not sure if this or just dropping a bunch of consts is better. We're essentially lying here when we do a const_cast...

aelovikov-intel · 2025-06-10T21:19:19Z

sycl/source/detail/scheduler/commands.cpp

+#ifdef __INTEL_PREVIEW_BREAKING_CHANGES
+    detail::kernel_bundle_impl *KernelBundleImplPtr,
+#else


Most things under source/detail could start using the raw-pointer version immediately, without this pre-processor dispatch.

Thanks, that's both more readable and faster.

Bensuo

Graph-related changes LGTM!

sycl/include/sycl/handler.hpp

aelovikov-intel · 2025-06-11T14:38:15Z

sycl/include/sycl/handler.hpp

@@ -1739,6 +1744,10 @@ class __SYCL_EXPORT handler {
  /// called.
  void setUserFacingNodeType(ext::oneapi::experimental::node_type Type);

+#ifdef __INTEL_PREVIEW_BREAKING_CHANGES


Why can't this be done now? Same in the next file.

Sure, done.

aelovikov-intel · 2025-06-11T14:39:42Z

sycl/source/detail/device_image_impl.hpp

@@ -619,7 +619,7 @@ class device_image_impl {

  std::shared_ptr<kernel_impl> tryGetSourceBasedKernel(
      std::string_view Name, const context &Context,
-      const std::shared_ptr<kernel_bundle_impl> &OwnerBundle,
+      std::shared_ptr<kernel_bundle_impl> &&OwnerBundle,


Why not raw pointer/reference instead? Is it because you want to keep the PR manageable in size and that subgraph of the codebase is left to a subsequent change?

No, that's because I don't understand lifetime constrains for kernel. I mean, after switching to raw pointers we must guarantee that kernel created from kernel_bundle only used when kernel_bundle is alive.

sycl::kernel_bundle<sycl::bundle_state::executable> kb = /*...*/; sycl::kernel k = kb.ext_oneapi_get_kernel("foo");

Is SYCL spec or sycl_ext_oneapi_kernel_compiler_spirv allow us to require this?

This constrain is important by kernel_bundle_impl::ext_oneapi_get_kernel(), for kernel_bundle_impl::tryGetKernel() there is no usage of kernel out out kernel_bundle scope. But having 2 verisons of device_image_impl::tryGetSourceBasedKernel() for shared_ptr and for raw pointers doesn't look attractive.

What I meant is that you'll shared_from_this in a place where copy is created before this change.

What I meant is that you'll shared_from_this in a place where copy is created before this change.

Yes, that's correct, previously shared_ptr was copied and now one is created with shared_from_this. And what do you suggest to do?

Alexandr-Konovalov#2

Please find the updated version.

I don't like the idea to make a hole in the encapsulation, but we should definitely take into account that C++11 move semantics is a difficult subject.

we should definitely take into account that C++11 move semantics is a difficult subject.

Exactly! No moves is just easier for everyone ;)

aelovikov-intel · 2025-06-11T14:42:19Z

sycl/source/detail/kernel_bundle_impl.hpp

-              DevImgImpl->tryGetSourceBasedKernel(Name, MContext, Self,
-                                                  DevImgImpl))
+              // move is performed only when SourceBasedKernel is not null
+          DevImgImpl->tryGetSourceBasedKernel(Name, MContext, std::move(Self),


And somewhat replying to my question above, I think it would make sense to modify tryGetSourceBasedKernel to accept raw reference as part of this PR. We'd be able to avoid the ugly Self that way.

Also, the current version is buggy - if we call these more than once during the loop then the second call is trying to std::move something that's already is in the "moved-out-from" state.

Also, the current version is buggy - if we call these more than once during the loop then the second call is trying to std::move something that's already is in the "moved-out-from" state.

Why? Self became empty only during tryGetSourceBasedKernel() call that return non-null. And when tryGetSourceBasedKernel() return non-null, there is no subsequent iteration. Probably I have to write more detailed comment.

aelovikov-intel · 2025-06-11T14:43:32Z

sycl/source/detail/kernel_bundle_impl.hpp

-        Kernel, detail::getSyclObjImpl(MContext), SelectedImage, Self, ArgMask,
-        SelectedImage->get_ur_program_ref(), CacheMutex);
+        Kernel, detail::getSyclObjImpl(MContext), SelectedImage,
+        shared_from_this(), ArgMask, SelectedImage->get_ur_program_ref(),


Ideally, kernel_impl should accept raw reference too, but I'm fine if that is left to a future change.

aelovikov-intel · 2025-06-11T14:44:24Z

sycl/source/detail/kernel_impl.cpp

@@ -39,7 +39,7 @@ kernel_impl::kernel_impl(ur_kernel_handle_t Kernel, ContextImplPtr Context,

 kernel_impl::kernel_impl(ur_kernel_handle_t Kernel, ContextImplPtr ContextImpl,
                         DeviceImageImplPtr DeviceImageImpl,
-                         KernelBundleImplPtr KernelBundleImpl,
+                         KernelBundleImplPtr &&KernelBundleImpl,


Well, since we need to make the change here anyway, then making it raw reference/pointer as part of this PR is preferrable.

aelovikov-intel · 2025-06-11T14:47:16Z

sycl/source/handler.cpp

+// returns nullptr if Insert is false
+detail::kernel_bundle_impl *
+handler::getOrInsertHandlerKernelBundlePtr(bool Insert) const {
+  if (!impl->MKernelBundle && Insert) {


I think inverting this and making early return for Insert == false would be more readable.

aelovikov-intel · 2025-06-11T14:49:20Z

sycl/source/handler.cpp

+          std::shared_ptr<detail::kernel_bundle_impl> ExecKernelBundleImpPtr =
+              detail::getSyclObjImpl(ExecKernelBundle);


Suggested change

std::shared_ptr<detail::kernel_bundle_impl> ExecKernelBundleImpPtr =

detail::getSyclObjImpl(ExecKernelBundle);

detail::kernel_bundle_impl &ExecKernelBundleImpPtr =

*detail::getSyclObjImpl(ExecKernelBundle);

I plan to update getSyclObjImpl to return a raw reference once I finish refactoring all the SYCL RT classes. Updating all the code as part of ongoing refactorings will make my later task of doing the change much easier.

If making this change would cause unnecessary complications, then it's fine to avoid it for now, but if the fix is simple and possible at this time, please do it.

Same below.

Again, I do not understand the lifetimes for the change you suggested. If ExecKernelBundleImpPtr became reference, why KernelBundleImpPtr is valid? ExecKernelBundle is out of scope,

Both ExecKernelBundle and ExecKernelBundleImpPtr are in the same scope, and ExecKernelBundleImpPtr is the inner one, so it will be destroyed before the ExecKernelBundle. IIUC, it's not the ExecKernelBundleImpPtr that extends any lifetime, but a copy of it created when calling something with it. We can have an explicit shared_from_this (either here or at the callee) to make it clear where that lifetime extension happens.

it's not the ExecKernelBundleImpPtr that extends any lifetime, but a copy of it created when calling something with it.

Exactly! The lifetime is extended, because we pass ExecKernelBundleImpPtr to setHandlerKernelBundle(), where it's saved. But, if I understand you correctly, suggest to eliminate shared_ptr here and use only raw pointers. Then, only way to extend the scope is to pass to setHandlerKernelBundle() shared_from_this from the raw pointer. That is probably correct, but why is it better then the version with explicit shared_ptr?

Because if we pass by raw ptr/ref, then we represent non-null property through type system. And if we pass everything by raw ptr/ref and avoid any passing of shared pointers (and only do shared_from_this when we do store a shared pointer to memory, like member data field), then it becomes impossible to accidentally create an unnecessary shared pointer copy.

If we do have some interfaces with raw ptr/rf and other with shared pointers, then we have two approaches and some people who are less experienced with the code base will make the wrong choice in new code, and will create unnecessary shared pointer copies.

Good point, explicit shared_from_this increases readability. Done.

Co-authored-by: aelovikov-intel <andrei.elovikov@intel.com>

…undle_impl

…bundle_impl` Follow-up for intel#18899. Also adds proper `private_tag` argument for the ctor added in intel#18949 that missed that.

…bundle_impl` Follow-up for intel#18899. Also adds proper `private_tag` argument for the ctor added in intel#18949 that missed that. Also pass `span` by value while on it.

Alexandr-Konovalov added 2 commits June 10, 2025 17:30

[SYCL] enable_shared_from_this for kernel_bundle_impl

06ee707

This allow us to get rid of passing self to kernel_bundle_impl methods and to return raw pointer to kernel bundle instead of shared_ptr.

Merge branch 'sycl' into Alexandr-Konovalov/shared_from_this-kernel_b…

1ad2b56

…undle_impl

Alexandr-Konovalov had a problem deploying to WindowsCILock June 10, 2025 15:42 — with GitHub Actions Error

Code formatting.

c6130ff

Alexandr-Konovalov had a problem deploying to WindowsCILock June 10, 2025 16:22 — with GitHub Actions Error

Code formatting.

ea89dd2

Alexandr-Konovalov had a problem deploying to WindowsCILock June 10, 2025 16:29 — with GitHub Actions Error

Code formatting.

a777b1e

Alexandr-Konovalov temporarily deployed to WindowsCILock June 10, 2025 16:36 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock June 10, 2025 17:19 — with GitHub Actions Inactive

aelovikov-intel self-requested a review June 10, 2025 21:13

aelovikov-intel reviewed Jun 10, 2025

View reviewed changes

Add getOrInsertHandlerKernelBundlePtr.

7974ec0

Alexandr-Konovalov had a problem deploying to WindowsCILock June 11, 2025 10:55 — with GitHub Actions Failure

Alexandr-Konovalov temporarily deployed to WindowsCILock June 11, 2025 11:23 — with GitHub Actions Inactive

Code formatting.

6aa90e9

Alexandr-Konovalov temporarily deployed to WindowsCILock June 11, 2025 12:03 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock June 11, 2025 12:25 — with GitHub Actions Inactive

Alexandr-Konovalov marked this pull request as ready for review June 11, 2025 12:32

Alexandr-Konovalov requested review from a team as code owners June 11, 2025 12:32

Alexandr-Konovalov requested review from Bensuo and againull June 11, 2025 12:32

Alexandr-Konovalov mentioned this pull request Jun 11, 2025

[SYCL] Return ref to shared_ptr from handler::getOrInsertHandlerKernelBundle() #18588

Draft

Bensuo approved these changes Jun 11, 2025

View reviewed changes

aelovikov-intel reviewed Jun 11, 2025

View reviewed changes

Update sycl/include/sycl/handler.hpp

4b651b6

Co-authored-by: aelovikov-intel <andrei.elovikov@intel.com>

Alexandr-Konovalov temporarily deployed to WindowsCILock June 13, 2025 15:10 — with GitHub Actions Inactive

Add newly exported Windows symbol.

47af740

Alexandr-Konovalov temporarily deployed to WindowsCILock June 13, 2025 15:32 — with GitHub Actions Inactive

Alexandr-Konovalov had a problem deploying to WindowsCILock June 13, 2025 16:22 — with GitHub Actions Failure

Alexandr-Konovalov temporarily deployed to WindowsCILock June 13, 2025 16:22 — with GitHub Actions Inactive

Alexandr-Konovalov added 2 commits June 16, 2025 14:23

Merge branch 'sycl' into Alexandr-Konovalov/shared_from_this-kernel_b…

60c1806

…undle_impl

Create std::shared_ptr<detail::kernel_bundle_impl> only on spot.

6c42146

Alexandr-Konovalov temporarily deployed to WindowsCILock June 16, 2025 13:57 — with GitHub Actions Inactive

Alexandr-Konovalov had a problem deploying to WindowsCILock June 16, 2025 14:22 — with GitHub Actions Error

Code formatting.

1e3f2d4

Alexandr-Konovalov temporarily deployed to WindowsCILock June 16, 2025 14:26 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock June 16, 2025 15:09 — with GitHub Actions Inactive

Do not use move semanics and call shared_from_this() on the spot.

35a8acf

Alexandr-Konovalov had a problem deploying to WindowsCILock June 17, 2025 10:35 — with GitHub Actions Error

Code formatting.

3f1efb2

Alexandr-Konovalov temporarily deployed to WindowsCILock June 17, 2025 10:53 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock June 17, 2025 11:20 — with GitHub Actions Inactive

Merge branch 'sycl' into Alexandr-Konovalov/shared_from_this-kernel_b…

b2f44c4

…undle_impl

Alexandr-Konovalov temporarily deployed to WindowsCILock June 17, 2025 12:00 — with GitHub Actions Inactive

Alexandr-Konovalov temporarily deployed to WindowsCILock June 17, 2025 12:55 — with GitHub Actions Inactive

aelovikov-intel approved these changes Jun 17, 2025

View reviewed changes

againull merged commit 74c7854 into intel:sycl Jun 17, 2025
37 of 39 checks passed

aelovikov-intel mentioned this pull request Jun 26, 2025

[NFC][SYCL] More cleanup after enable_shared_from_this for kernel_bundle_impl #19185

Open

		std::shared_ptr<kernel_bundle_impl> Self =
		const_cast<kernel_bundle_impl *>(this)->shared_from_this();

		std::shared_ptr<detail::kernel_bundle_impl> ExecKernelBundleImpPtr =
		detail::getSyclObjImpl(ExecKernelBundle);

[SYCL] enable_shared_from_this for kernel_bundle_impl #18899

[SYCL] enable_shared_from_this for kernel_bundle_impl #18899

Uh oh!

Conversation

Alexandr-Konovalov commented Jun 10, 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!

Bensuo left a comment

Choose a reason for hiding this comment

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!

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!

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!

Uh oh!

Uh oh!