From 13444292f0a2f792c5127bdc771512aac8bc6384 Mon Sep 17 00:00:00 2001 From: Lukasz Dorau Date: Fri, 12 Sep 2025 11:26:45 +0000 Subject: [PATCH 1/2] [SYCL] Do not call urDeviceRetain() in case of subdevices urDeviceRetain(MDevice) should not be called in the constructor of device_impl in case of subdevices, because their reference counter does not drop to zero and subdevices are not destroyed nor freed now, what causes memory leaks. It fixes URT-961. Signed-off-by: Lukasz Dorau --- sycl/source/detail/device_impl.cpp | 12 +++++++----- sycl/source/detail/device_impl.hpp | 5 +++-- sycl/source/detail/platform_impl.cpp | 16 +++++++++++++++- sycl/source/detail/platform_impl.hpp | 11 +++++++++++ 4 files changed, 36 insertions(+), 8 deletions(-) diff --git a/sycl/source/detail/device_impl.cpp b/sycl/source/detail/device_impl.cpp index 629aa72f04dde..214612fe23e8c 100644 --- a/sycl/source/detail/device_impl.cpp +++ b/sycl/source/detail/device_impl.cpp @@ -22,7 +22,7 @@ namespace detail { /// Constructs a SYCL device instance using the provided /// UR device instance. device_impl::device_impl(ur_device_handle_t Device, platform_impl &Platform, - device_impl::private_tag) + device_impl::private_tag, bool IsSubDevice) : MDevice(Device), MPlatform(Platform), // No need to set MRootDevice when MAlwaysRootDevice is true MRootDevice(Platform.MAlwaysRootDevice @@ -30,9 +30,11 @@ device_impl::device_impl(ur_device_handle_t Device, platform_impl &Platform, : get_info_impl()), // TODO catch an exception and put it to list of asynchronous exceptions: MCache{*this} { - // Interoperability Constructor already calls DeviceRetain in - // urDeviceCreateWithNativeHandle. - getAdapter().call(MDevice); + if (!IsSubDevice) { + // Interoperability Constructor already calls DeviceRetain in + // urDeviceCreateWithNativeHandle. + getAdapter().call(MDevice); + } } device_impl::~device_impl() { @@ -164,7 +166,7 @@ std::vector device_impl::create_sub_devices( std::for_each(SubDevices.begin(), SubDevices.end(), [&res, this](const ur_device_handle_t &a_ur_device) { device sycl_device = detail::createSyclObjFromImpl( - MPlatform.getOrMakeDeviceImpl(a_ur_device)); + MPlatform.getOrMakeSubDeviceImpl(a_ur_device)); res.push_back(sycl_device); }); return res; diff --git a/sycl/source/detail/device_impl.hpp b/sycl/source/detail/device_impl.hpp index 38214254595c6..d5f80ef5ab421 100644 --- a/sycl/source/detail/device_impl.hpp +++ b/sycl/source/detail/device_impl.hpp @@ -429,10 +429,11 @@ class device_impl : public std::enable_shared_from_this { /// Constructs a SYCL device instance using the provided /// UR device instance. // - // Must be called through `platform_impl::getOrMakeDeviceImpl` only. + // Must be called through `platform_impl::getOrMakeDeviceImpl` or + // `platform_impl::getOrMakeSubDeviceImpl` only. // `private_tag` ensures that is true. explicit device_impl(ur_device_handle_t Device, platform_impl &Platform, - private_tag); + private_tag, bool IsSubDevice); ~device_impl(); diff --git a/sycl/source/detail/platform_impl.cpp b/sycl/source/detail/platform_impl.cpp index a60f20d5dd78e..b187b57c28d4e 100644 --- a/sycl/source/detail/platform_impl.cpp +++ b/sycl/source/detail/platform_impl.cpp @@ -306,7 +306,21 @@ device_impl &platform_impl::getOrMakeDeviceImpl(ur_device_handle_t UrDevice) { // Otherwise make the impl MDevices.emplace_back(std::make_shared( - UrDevice, *this, device_impl::private_tag{})); + UrDevice, *this, device_impl::private_tag{}, false /*IsSubDevice*/)); + + return *MDevices.back(); +} + +device_impl & +platform_impl::getOrMakeSubDeviceImpl(ur_device_handle_t UrSubDevice) { + const std::lock_guard Guard(MDeviceMapMutex); + // If we've already seen this device, return the impl + if (device_impl *Result = getDeviceImplHelper(UrSubDevice)) + return *Result; + + // Otherwise make the impl + MDevices.emplace_back(std::make_shared( + UrSubDevice, *this, device_impl::private_tag{}, true /*IsSubDevice*/)); return *MDevices.back(); } diff --git a/sycl/source/detail/platform_impl.hpp b/sycl/source/detail/platform_impl.hpp index adc2cb6c04a9a..81866867cbbb2 100644 --- a/sycl/source/detail/platform_impl.hpp +++ b/sycl/source/detail/platform_impl.hpp @@ -172,6 +172,17 @@ class platform_impl : public std::enable_shared_from_this { /// \return a device_impl* corresponding to the device device_impl &getOrMakeDeviceImpl(ur_device_handle_t UrDevice); + /// Queries the device_impl cache to either return a shared_ptr + /// for the device_impl corresponding to the UrSubDevice or add + /// a new entry to the cache + /// + /// \param UrSubDevice is the UrSubDevice whose impl is requested + /// + /// \param PlatormImpl is the Platform for that Device + /// + /// \return a device_impl* corresponding to the subdevice + device_impl &getOrMakeSubDeviceImpl(ur_device_handle_t UrSubDevice); + /// Queries the cache to see if the specified UR platform has been seen /// before. If so, return the cached platform_impl, otherwise create a new /// one and cache it. From 119f6a2b2ede47c3b423806bd4b4ab17f1406bd9 Mon Sep 17 00:00:00 2001 From: Lukasz Dorau Date: Tue, 16 Sep 2025 09:26:34 +0000 Subject: [PATCH 2/2] Unify getOrMakeSubDeviceImpl with getOrMakeDeviceImpl --- sycl/source/detail/device_impl.cpp | 13 +++++++------ sycl/source/detail/device_impl.hpp | 3 +-- sycl/source/detail/platform_impl.cpp | 19 +++---------------- sycl/source/detail/platform_impl.hpp | 14 ++------------ 4 files changed, 13 insertions(+), 36 deletions(-) diff --git a/sycl/source/detail/device_impl.cpp b/sycl/source/detail/device_impl.cpp index 214612fe23e8c..28bb50009c0de 100644 --- a/sycl/source/detail/device_impl.cpp +++ b/sycl/source/detail/device_impl.cpp @@ -163,12 +163,13 @@ std::vector device_impl::create_sub_devices( // times with the same arguments? // std::vector res; - std::for_each(SubDevices.begin(), SubDevices.end(), - [&res, this](const ur_device_handle_t &a_ur_device) { - device sycl_device = detail::createSyclObjFromImpl( - MPlatform.getOrMakeSubDeviceImpl(a_ur_device)); - res.push_back(sycl_device); - }); + std::for_each( + SubDevices.begin(), SubDevices.end(), + [&res, this](const ur_device_handle_t &a_ur_device) { + device sycl_device = detail::createSyclObjFromImpl( + MPlatform.getOrMakeDeviceImpl(a_ur_device, true /*IsSubDevice*/)); + res.push_back(sycl_device); + }); return res; } diff --git a/sycl/source/detail/device_impl.hpp b/sycl/source/detail/device_impl.hpp index d5f80ef5ab421..4d6313d729f97 100644 --- a/sycl/source/detail/device_impl.hpp +++ b/sycl/source/detail/device_impl.hpp @@ -429,8 +429,7 @@ class device_impl : public std::enable_shared_from_this { /// Constructs a SYCL device instance using the provided /// UR device instance. // - // Must be called through `platform_impl::getOrMakeDeviceImpl` or - // `platform_impl::getOrMakeSubDeviceImpl` only. + // Must be called through `platform_impl::getOrMakeDeviceImpl` only. // `private_tag` ensures that is true. explicit device_impl(ur_device_handle_t Device, platform_impl &Platform, private_tag, bool IsSubDevice); diff --git a/sycl/source/detail/platform_impl.cpp b/sycl/source/detail/platform_impl.cpp index b187b57c28d4e..db72fcb22f05e 100644 --- a/sycl/source/detail/platform_impl.cpp +++ b/sycl/source/detail/platform_impl.cpp @@ -298,7 +298,8 @@ device_impl *platform_impl::getDeviceImpl(ur_device_handle_t UrDevice) { return getDeviceImplHelper(UrDevice); } -device_impl &platform_impl::getOrMakeDeviceImpl(ur_device_handle_t UrDevice) { +device_impl &platform_impl::getOrMakeDeviceImpl(ur_device_handle_t UrDevice, + bool IsSubDevice) { const std::lock_guard Guard(MDeviceMapMutex); // If we've already seen this device, return the impl if (device_impl *Result = getDeviceImplHelper(UrDevice)) @@ -306,21 +307,7 @@ device_impl &platform_impl::getOrMakeDeviceImpl(ur_device_handle_t UrDevice) { // Otherwise make the impl MDevices.emplace_back(std::make_shared( - UrDevice, *this, device_impl::private_tag{}, false /*IsSubDevice*/)); - - return *MDevices.back(); -} - -device_impl & -platform_impl::getOrMakeSubDeviceImpl(ur_device_handle_t UrSubDevice) { - const std::lock_guard Guard(MDeviceMapMutex); - // If we've already seen this device, return the impl - if (device_impl *Result = getDeviceImplHelper(UrSubDevice)) - return *Result; - - // Otherwise make the impl - MDevices.emplace_back(std::make_shared( - UrSubDevice, *this, device_impl::private_tag{}, true /*IsSubDevice*/)); + UrDevice, *this, device_impl::private_tag{}, IsSubDevice)); return *MDevices.back(); } diff --git a/sycl/source/detail/platform_impl.hpp b/sycl/source/detail/platform_impl.hpp index 81866867cbbb2..dc8cc8866f5ef 100644 --- a/sycl/source/detail/platform_impl.hpp +++ b/sycl/source/detail/platform_impl.hpp @@ -170,18 +170,8 @@ class platform_impl : public std::enable_shared_from_this { /// \param PlatormImpl is the Platform for that Device /// /// \return a device_impl* corresponding to the device - device_impl &getOrMakeDeviceImpl(ur_device_handle_t UrDevice); - - /// Queries the device_impl cache to either return a shared_ptr - /// for the device_impl corresponding to the UrSubDevice or add - /// a new entry to the cache - /// - /// \param UrSubDevice is the UrSubDevice whose impl is requested - /// - /// \param PlatormImpl is the Platform for that Device - /// - /// \return a device_impl* corresponding to the subdevice - device_impl &getOrMakeSubDeviceImpl(ur_device_handle_t UrSubDevice); + device_impl &getOrMakeDeviceImpl(ur_device_handle_t UrDevice, + bool IsSubDevice = false); /// Queries the cache to see if the specified UR platform has been seen /// before. If so, return the cached platform_impl, otherwise create a new