Skip to content

Conversation

@aelovikov-intel
Copy link
Contributor

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

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::devices 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 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 verying that platform isn't
empty are preserved inside the `platform_impl` object and this existing
SYCL devices hierarcy is used when creating user's device object.
@aelovikov-intel aelovikov-intel merged commit 7b8996e into intel:sycl Apr 30, 2025
39 of 43 checks passed
@aelovikov-intel aelovikov-intel deleted the keep-devices-alive branch April 30, 2025 15:45
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 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.
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.
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