-
Notifications
You must be signed in to change notification settings - Fork 788
[SYCL] Less shared_ptr
for platform_impl
#18143
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
Conversation
d505db8
to
696c933
Compare
1e8d5a9
to
6964b35
Compare
6964b35
to
04c2ad7
Compare
04c2ad7
to
e27ad79
Compare
e27ad79
to
2ed9b26
Compare
2ed9b26
to
e588f8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the SYCL runtime to reduce unnecessary use of std::shared_ptr for platform_impl objects by replacing them with raw references or pointers in critical parts of the code, thereby reducing ownership overhead. The changes update several interfaces and call sites throughout the codebase.
- Changed the platform interface in tests and source files (e.g. printers.cpp, platform.cpp) to use platform_impl references.
- Updated device and program manager implementations to adjust method calls and data access to the new raw reference API.
- Refactored platform_impl.hpp/.cpp and related files to expose getSharedPtrToSelf() and return references instead of smart pointers.
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
sycl/test/gdb/printers.cpp | Updated comments to reflect the change from shared_ptr to references for platform_impl. |
sycl/source/platform.cpp | Modified platform_impl acquisition to return a shared pointer via getSharedPtrToSelf() and updated related call sites. |
sycl/source/device.cpp | Adjusted device_impl initialization to use the new platform_impl API with references. |
sycl/source/detail/usm/usm_impl.cpp | Replaced arrow operator calls with dot operator calls on platform_impl references. |
sycl/source/detail/program_manager/program_manager.cpp | Refactored access to platform_impl from pointer to reference in option handling and backend option retrieval. |
sycl/source/detail/platform_impl.hpp/.cpp | Changed several methods in platform_impl to return references instead of shared_ptr, including updates to helper methods and caching. |
Other Files (global_handler.hpp/.cpp, device_impl.hpp/.cpp, context_impl.hpp/.cpp, backend files, etc.) | Updated all affected call sites and interface definitions to conform with the new ownership model. |
e588f8f
to
f12b017
Compare
26cb6e7
to
70a2a8f
Compare
shared_ptr
for platform_impl
shared_ptr
for platform_impl
70a2a8f
to
5b0eaa8
Compare
5b0eaa8
to
a26c4fe
Compare
…8251) After that devices are never destroyed until the SYCL RT library shutdown. In practice, that means that before the change a simple ``` int main() { sycl::device d; } ``` went into `platform` ctor, then queried all the platform's devices to check that it has some, returned from ctor and those `sycl::device`s created on stack were already destroyed. After that, when creating user's `sycl::device d` we were re-creating device hierarchy for the platform at SYCL level again (including some calls to `urDeviceGetInfo` during `device_impl` creation). After the changes, devices created when veryfing that platform isn't empty are preserved inside the `platform_impl` object and this existing SYCL devices hierarchy is used when creating user's device object. A note on the implementation: `device_impl` has an `std::shared_ptr<platform_impl>` inside so we can't rely on automatic resource management just by the nature of `std::shared_ptr` everywhere (and we haven't changed this aspect in #18143). As such, we have to perform some explicit resource release during shutdown procedure (or in `~UrMock()` for unittests).
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.
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_this` and keep creating shared pointers for member objects to keep the graph of resource ownership intact.
GlobalHandler::MPlatformCache
keeps (shared) ownership ofplatform_impl
objects, so none of them could be destroyed until SYCL RT library shutdown/unload process. As such, using raw pointers/reference toplatform_impl
throughout the SYCL RT is totally fine and would avoid extra costs ofstd::shared_ptr
. However, our unittests don't work the same way and lifetimes are actually managed by all the data memberstd::shared_ptr
s (e.g., seeCurrentDevice.cpp
as the most obvious case), so simply switching allPlatformImplPtr
to raw pointer/reference doesn't work right now. I think it will be possible with an ABI break if we'd remove all the shared pointers but the one in theGlobalHandler
.What I'm doing here instead is the following:
std::enable_shared_from_this()
so that we could pass raw references around and only actually create smart pointers when absolutely necessary.This should make it much harder to write something like this: