Skip to content

Conversation

@aelovikov-intel
Copy link
Contributor

@aelovikov-intel aelovikov-intel commented Apr 22, 2025

GlobalHandler::MPlatformCache keeps (shared) ownership of platform_impl objects, so none of them could be destroyed until SYCL RT library shutdown/unload process. As such, using raw pointers/reference to platform_impl throughout the SYCL RT is totally fine and would avoid extra costs of std::shared_ptr. However, our unittests don't work the same way and lifetimes are actually managed by all the data member std::shared_ptrs (e.g., see CurrentDevice.cpp as the most obvious case), so simply switching all PlatformImplPtr 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 the GlobalHandler.

What I'm doing here instead is the following:

  • Try to keep lifetime management the same, i.e., all data member pointers are still "smart"
  • Inherit from std::enable_shared_from_this() so that we could pass raw references around and only actually create smart pointers when absolutely necessary.
  • Change internal APIs to operate more using plain references

This should make it much harder to write something like this:

  // BAD: Should have been `auto &`, extra atomic counter
  // increment/decrement without it.
  auto PlatformImpl = d.getPlatformImplPtr();

@uditagarwal97 uditagarwal97 requested a review from Copilot April 24, 2025 17:52
Copy link
Contributor

Copilot AI left a 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.

@againull againull merged commit 73535d9 into intel:sycl Apr 28, 2025
25 checks passed
@aelovikov-intel aelovikov-intel deleted the platform_impl branch April 28, 2025 22:45
aelovikov-intel added a commit that referenced this pull request Apr 30, 2025
…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).
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Apr 30, 2025
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 added a commit that referenced this pull request May 1, 2025
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.
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 28, 2025
Refactoring has started in
intel#18143
intel#18251
but this final step wasn't performed before due to some unittests
failing. Seems not to be the case anymore.
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 28, 2025
Refactoring has started in
intel#18143
intel#18251
but this final step wasn't performed before due to some unittests
failing. Seems not to be the case anymore.
aelovikov-intel added a commit that referenced this pull request Jul 29, 2025
Refactoring has started in
#18143
#18251
but this final step wasn't performed before due to some unittests
failing. Seems not to be the case anymore.
aelovikov-intel added a commit to aelovikov-intel/llvm that referenced this pull request Jul 29, 2025
Similar to intel#19613.

Refactoring has started in
intel#18143
intel#18251

This didn't work back then due to some unittests failing (because
`global_handler` shutdown is different with unittests) but it doesn't
seem to be an issue any more, probably due to other refactoring PRs that
prefer raw ptr/refs over `std::shared_ptr` elsewhere.
aelovikov-intel added a commit that referenced this pull request Aug 1, 2025
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.

2 participants