[SYCL][NFCI] Less shared_ptr for device_impl
#18270
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After intel#18251 device are guaranteed to be alive until SYCL RT library shutdown, so we don't have to pass everything in `std::shared_ptr<device_impl>` and might use raw pointers/references much more. That said, constraints from intel#18143 (mostly unittests linking statically and lifetimes of static/thread-local objects following from that) are still here and I'm addressing them the same way - not totally changing the ownership model, using `std::enable_shared_from_this` and keep creating shared pointers for member objects to keep the graph of resource ownership intact.
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
jopperm
approved these changes
Apr 30, 2025
Contributor
jopperm
left a comment
jopperm
left a comment
There was a problem hiding this comment.
Change to sycl/source/detail/jit_compiler.cpp LGTM.
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
commented
Apr 30, 2025
| MNextAvailableQueueID.fetch_add(1, std::memory_order_relaxed)} { | ||
| queue_impl_interop(UrQueue); | ||
| } | ||
| : queue_impl(UrQueue, Context, AsyncHandler, {}) {} |
Contributor
Author
There was a problem hiding this comment.
Using delegating ctor here instead of non-ctor helper queue_impl_interop (that was inlined into the ctor below).
aelovikov-intel
commented
Apr 30, 2025
| "Host task submissions should have an associated queue"); | ||
| interop_handle IH{MReqToMem, HostTask.MQueue, | ||
| HostTask.MQueue->getDeviceImplPtr(), | ||
| HostTask.MQueue->getDeviceImpl().shared_from_this(), |
Contributor
Author
There was a problem hiding this comment.
interop_handle seems to be part of ABI, so not changing it here.
aelovikov-intel
commented
Apr 30, 2025
Comment on lines
-40
to
-42
| RTDeviceBinaryImage * | ||
| retrieveAMDGCNOrNVPTXKernelBinary(const DeviceImplPtr DeviceImpl, | ||
| const std::string &KernelName); |
Contributor
Author
There was a problem hiding this comment.
Seems dead, just removing.
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
keyradical
approved these changes
May 1, 2025
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Contributor
Author
|
@uditagarwal97 , ping. |
uditagarwal97
approved these changes
May 1, 2025
aelovikov-intel
changed the title
shared_ptr for device_implshared_ptr for device_impl
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
pushed a commit
that referenced
this pull request
May 5, 2025
Refactored the `ProgramManager` to use `device_impl &` instead of `const device &`. See #18270 and #18251 that started the refactoring. Signed-off-by: Sergei Vinogradov <[email protected]>
aelovikov-intel
added a commit
to aelovikov-intel/llvm
that referenced
this pull request
Jul 15, 2025
…e_impl *` intel#18251 extended `device_impl`s' lifetimes until shutdown and intel#18270 started to pass devices as raw pointers in some of the APIs. This PR builds on top of that and extends usage of raw pointers/references/`device_range` as the devices are known to be alive and extra `std::shared_ptr`'s atomic increments aren't necessary and could be avoided. This change mostly touches `device_image_impl` and `program_manager` and switches most of the APIs to use `devices_range`. A few number of other modifications are caused by these APIs' changes and are necessary to keep the code buildable.
aelovikov-intel
added a commit
to aelovikov-intel/llvm
that referenced
this pull request
Jul 15, 2025
…e_impl *` intel#18251 extended `device_impl`s' lifetimes until shutdown and intel#18270 started to pass devices as raw pointers in some of the APIs. This PR builds on top of that and extends usage of raw pointers/references/`device_range` as the devices are known to be alive and extra `std::shared_ptr`'s atomic increments aren't necessary and could be avoided. This change mostly touches `device_image_impl` and `program_manager` and switches most of the APIs to use `devices_range`. A few number of other modifications are caused by these APIs' changes and are necessary to keep the code buildable.
againull
pushed a commit
that referenced
this pull request
Jul 16, 2025
…e_impl *` (#19459) #18251 extended `device_impl`s' lifetimes until shutdown and #18270 started to pass devices as raw pointers in some of the APIs. This PR builds on top of that and extends usage of raw pointers/references/`device_range` as the devices are known to be alive and extra `std::shared_ptr`'s atomic increments aren't necessary and could be avoided. Since we change the type of `device_image_impl::MDevices`, other APIs in that class and in `program_manager` don't need to operate in terms of `sycl::device` or `std::shared_ptr<device_impl>` and we can switch them to use `devices_range` instead. A small number of other modifications are caused by these APIs' changes and are necessary to keep the code buildable. One extra change is the addition of a minor `devices_range::to<std::vector<ur_device_handle_t>>()` helper that we can use now that most of the arguments are `device_range`. Technically, could go in another PR but then we'd just be modifying the exact same lines two times, so I decided to fuse it here.
aelovikov-intel
added a commit
to aelovikov-intel/llvm
that referenced
this pull request
Jul 16, 2025
…ce_impl *` intel#18251 extended `device_impl`s' lifetimes until shutdown and intel#18270 started to pass devices as raw pointers in some of the APIs. This PR builds on top of that and extends usage of raw pointers/references/`device_range` as the devices are known to be alive and extra `std::shared_ptr`'s atomic increments aren't necessary and could be avoided. Since we change the type of `kernel_bundle_impl::MDevices`, other APIs in that class don't need to operate in terms of `sycl::device` or `std::shared_ptr<device_impl>` and we can switch them to use `devices_range` instead. A small number of other modifications are caused by these APIs' changes and are necessary to keep the code buildable.
aelovikov-intel
added a commit
to aelovikov-intel/llvm
that referenced
this pull request
Jul 16, 2025
…ce_impl *` intel#18251 extended `device_impl`s' lifetimes until shutdown and intel#18270 started to pass devices as raw pointers in some of the APIs. This PR builds on top of that and extends usage of raw pointers/references/`device_range` as the devices are known to be alive and extra `std::shared_ptr`'s atomic increments aren't necessary and could be avoided. Since we change the type of `kernel_bundle_impl::MDevices`, other APIs in that class don't need to operate in terms of `sycl::device` or `std::shared_ptr<device_impl>` and we can switch them to use `devices_range` instead. A small number of other modifications are caused by these APIs' changes and are necessary to keep the code buildable.
aelovikov-intel
added a commit
to aelovikov-intel/llvm
that referenced
this pull request
Jul 16, 2025
…ce_impl *` intel#18251 extended `device_impl`s' lifetimes until shutdown and intel#18270 started to pass devices as raw pointers in some of the APIs. This PR builds on top of that and extends usage of raw pointers/references/`device_range` as the devices are known to be alive and extra `std::shared_ptr`'s atomic increments aren't necessary and could be avoided. Since we change the type of `kernel_bundle_impl::MDevices`, other APIs in that class don't need to operate in terms of `sycl::device` or `std::shared_ptr<device_impl>` and we can switch them to use `devices_range` instead. A small number of other modifications are caused by these APIs' changes and are necessary to keep the code buildable.
aelovikov-intel
added a commit
that referenced
this pull request
Jul 17, 2025
…ce_impl *` (#19484) #18251 extended `device_impl`s' lifetimes until shutdown and #18270 started to pass devices as raw pointers in some of the APIs. This PR builds on top of that and extends usage of raw pointers/references/`device_range` as the devices are known to be alive and extra `std::shared_ptr`'s atomic increments aren't necessary and could be avoided. Since we change the type of `kernel_bundle_impl::MDevices`, other APIs in that class don't need to operate in terms of `sycl::device` or `std::shared_ptr<device_impl>` and we can switch them to use `devices_range` instead. A small number of other modifications are caused by these APIs' changes and are necessary to keep the code buildable.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After #18251 devices are guaranteed to be alive until SYCL RT library shutdown, so we don't have to pass everything in
std::shared_ptr<device_impl>and might use raw pointers/references much more.That said, constraints from
#18143 (mostly unittests linking statically and lifetimes of static/thread-local objects following from that) are still here and I'm addressing them the same way - not totally changing the ownership model, using
std::enable_shared_from_thisand keep creating shared pointers for member objects to keep the graph of resource ownership intact.