[SYCL] piDeviceRelease resource leak fix #2668
Conversation
…nd platform_impl, each holding a shared pointer to the other, preventing their destruction. Here device_impl is modified to use a weak_ptr to the platform_impl, but this means the device must track the plugin as well, as it needs it during its destruction (when the weak_ptr<platform> will be dangling). Signed-off-by: Chris Perkins <[email protected]>
| @@ -101,7 +104,10 @@ cl_device_id device_impl::get() const { | |||
| } | |||
|
|
|||
| platform device_impl::get_platform() const { | |||
| return createSyclObjFromImpl<platform>(MPlatform); | |||
| if (PlatformImplPtr Platform = MPlatform.lock()) | |||
There was a problem hiding this comment.
I think it's not very clear what should be done if a platform is released before a device bound to this platform.
Can we do vice versa - device has a shared_ptr to platform, platform has a weak_ptr to device?
There was a problem hiding this comment.
I can try it.
The platform has multiple owners "higher up", like the context and the GlobalHandler. So it isn't affected if the device has a weak_ptr to it. But I'm less certain for the devices. I think the platform might be the primary owner, so if it's a weak pointer, then the device might be released before the platform is done with it. Also, the code has a clear chain of ownership now: context->platform->device, it seems unwise to meddle with it.
I would prefer that instead of returning just platform() in the else case, that we throw an exception - because no one should be asking for the platform after it has been released. However, that would be an ABI change, and I wanted to avoid anything controversial.
There was a problem hiding this comment.
I changed this PR to draft, and opened a new one here: #2671
OpenCL spec supports atomic_float/atomic_double type for atomic_compare_exchange* functions. However, value and return type in OpAtomicCompareExchange in SPIR-V spec must be integer type. Therefore, in OCLToSPIRV translation we need to translate floating-point type to corresponding integer variant that has the same type size. Floating-point value is bitcasted so that bits remain the same. Original commit: KhronosGroup/SPIRV-LLVM-Translator@e5544014fba77d3
piDeviceRetain/piDeviceRelease fix.
There is a dependency loop between device_impl and platform_impl, each holding a shared pointer to the other, preventing their destruction. Here device_impl is modified to use a weak_ptr to the platform_impl, but this means the device must track the plugin as well, as it needs it during its destruction (when the weak_ptr will be dangling).
Once device_impl destructor runs, piDeviceRelease is called.
Signed-off-by: Chris Perkins [email protected]