From c538618fb3bb609ef7b430fc034ddc15cf7d7ced Mon Sep 17 00:00:00 2001 From: "Li, Ian" Date: Mon, 14 Apr 2025 13:07:26 -0700 Subject: [PATCH 1/6] Enforce access to m_DeviceImages using m_KernelIDsMutex --- sycl/source/detail/program_manager/program_manager.cpp | 7 ++++--- sycl/source/detail/program_manager/program_manager.hpp | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index cbd590bd686b5..58eb2ae4e0597 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -2148,6 +2148,10 @@ void ProgramManager::addImages(sycl_device_binaries DeviceBinary) { void ProgramManager::removeImages(sycl_device_binaries DeviceBinary) { for (int I = 0; I < DeviceBinary->NumDeviceBinaries; I++) { sycl_device_binary RawImg = &(DeviceBinary->DeviceBinaries[I]); + + // Acquire lock to read and modify maps for kernel bundles + std::lock_guard KernelIDsGuard(m_KernelIDsMutex); + auto DevImgIt = m_DeviceImages.find(RawImg); if (DevImgIt == m_DeviceImages.end()) continue; @@ -2161,9 +2165,6 @@ void ProgramManager::removeImages(sycl_device_binaries DeviceBinary) { // Drop the kernel argument mask map m_EliminatedKernelArgMasks.erase(Img); - // Acquire lock to modify maps for kernel bundles - std::lock_guard KernelIDsGuard(m_KernelIDsMutex); - // Unmap the unique kernel IDs for the offload entries for (sycl_offload_entry EntriesIt = EntriesB; EntriesIt != EntriesE; EntriesIt = EntriesIt->Increment()) { diff --git a/sycl/source/detail/program_manager/program_manager.hpp b/sycl/source/detail/program_manager/program_manager.hpp index 906692cdcac25..c43b89cec5c84 100644 --- a/sycl/source/detail/program_manager/program_manager.hpp +++ b/sycl/source/detail/program_manager/program_manager.hpp @@ -460,6 +460,7 @@ class ProgramManager { /// Keeps all device images we are refering to during program lifetime. Used /// for proper cleanup. + /// Access must be guarded by the m_KernelIDsMutex mutex. std::unordered_map m_DeviceImages; From 9a7e249eebfbee33d891da7551def09aa1854cda Mon Sep 17 00:00:00 2001 From: "Li, Ian" Date: Mon, 21 Apr 2025 11:12:55 -0700 Subject: [PATCH 2/6] Enforce access to m_VFSet2BinImage using m_KernelIDsMutex --- .../program_manager/program_manager.cpp | 59 +++++++++++-------- .../program_manager/program_manager.hpp | 1 + 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 58eb2ae4e0597..7586af8f429b8 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -756,35 +756,42 @@ ProgramManager::collectDependentDeviceImagesForVirtualFunctions( } } - while (!WorkList.empty()) { - std::string SetName = WorkList.front(); - WorkList.pop(); + { + // Guard read access to m_VFSet2BinImage: + // TODO: a better solution should be sought in the future, i.e. a different + // mutex than m_KernelIDsMutex, check lock check pattern, etc. + std::lock_guard KernelIDsGuard(m_KernelIDsMutex); - // There could be more than one device image that uses the same set - // of virtual functions, or provides virtual funtions from the same - // set. - for (RTDeviceBinaryImage *BinImage : m_VFSet2BinImage[SetName]) { - // Here we can encounter both uses-virtual-functions-set and - // virtual-functions-set properties, but their handling is the same: we - // just grab all sets they reference and add them for consideration if - // we haven't done so already. - for (const sycl_device_binary_property &VFProp : - BinImage->getVirtualFunctions()) { - std::string StrValue = DeviceBinaryProperty(VFProp).asCString(); - for (const auto &SetName : detail::split_string(StrValue, ',')) { - if (HandledSets.insert(SetName).second) - WorkList.push(SetName); + while (!WorkList.empty()) { + std::string SetName = WorkList.front(); + WorkList.pop(); + + // There could be more than one device image that uses the same set + // of virtual functions, or provides virtual funtions from the same + // set. + for (RTDeviceBinaryImage *BinImage : m_VFSet2BinImage.at(SetName)) { + // Here we can encounter both uses-virtual-functions-set and + // virtual-functions-set properties, but their handling is the same: we + // just grab all sets they reference and add them for consideration if + // we haven't done so already. + for (const sycl_device_binary_property &VFProp : + BinImage->getVirtualFunctions()) { + std::string StrValue = DeviceBinaryProperty(VFProp).asCString(); + for (const auto &SetName : detail::split_string(StrValue, ',')) { + if (HandledSets.insert(SetName).second) + WorkList.push(SetName); + } } - } - // TODO: Complete this part about handling of incompatible device images. - // If device image uses the same virtual function set, then we only - // link it if it is compatible. - // However, if device image provides virtual function set and it is - // incompatible, then we should link its "dummy" version to avoid link - // errors about unresolved external symbols. - if (doesDevSupportDeviceRequirements(Dev, *BinImage)) - DeviceImagesToLink.insert(BinImage); + // TODO: Complete this part about handling of incompatible device images. + // If device image uses the same virtual function set, then we only + // link it if it is compatible. + // However, if device image provides virtual function set and it is + // incompatible, then we should link its "dummy" version to avoid link + // errors about unresolved external symbols. + if (doesDevSupportDeviceRequirements(Dev, *BinImage)) + DeviceImagesToLink.insert(BinImage); + } } } diff --git a/sycl/source/detail/program_manager/program_manager.hpp b/sycl/source/detail/program_manager/program_manager.hpp index c43b89cec5c84..35843ea24def5 100644 --- a/sycl/source/detail/program_manager/program_manager.hpp +++ b/sycl/source/detail/program_manager/program_manager.hpp @@ -470,6 +470,7 @@ class ProgramManager { /// Caches list of device images that use or provide virtual functions from /// the same set. Used to simplify access. + /// Access must be guarded by the m_KernelIDsMutex mutex. std::unordered_map> m_VFSet2BinImage; From 0b2ce758059cb34fd6f5a9470e879bc6092484c1 Mon Sep 17 00:00:00 2001 From: "Li, Ian" Date: Mon, 21 Apr 2025 11:34:25 -0700 Subject: [PATCH 3/6] Apply clang-format --- .../detail/program_manager/program_manager.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 7586af8f429b8..2c35f1bba2d5e 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -783,12 +783,12 @@ ProgramManager::collectDependentDeviceImagesForVirtualFunctions( } } - // TODO: Complete this part about handling of incompatible device images. - // If device image uses the same virtual function set, then we only - // link it if it is compatible. - // However, if device image provides virtual function set and it is - // incompatible, then we should link its "dummy" version to avoid link - // errors about unresolved external symbols. + // TODO: Complete this part about handling of incompatible device + // images. If device image uses the same virtual function set, then we + // only link it if it is compatible. However, if device image provides + // virtual function set and it is incompatible, then we should link its + // "dummy" version to avoid link errors about unresolved external + // symbols. if (doesDevSupportDeviceRequirements(Dev, *BinImage)) DeviceImagesToLink.insert(BinImage); } From 9685cc733ae9648045a2b9acd1eb8766037e535b Mon Sep 17 00:00:00 2001 From: "Li, Ian" Date: Wed, 23 Apr 2025 12:04:48 -0700 Subject: [PATCH 4/6] Move lock out of removeImages action loop --- sycl/source/detail/program_manager/program_manager.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 2c35f1bba2d5e..bfe430d00afde 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -2153,12 +2153,13 @@ void ProgramManager::addImages(sycl_device_binaries DeviceBinary) { } void ProgramManager::removeImages(sycl_device_binaries DeviceBinary) { + if (DeviceBinary->NumDeviceBinaries == 0) return; + // Acquire lock to read and modify maps for kernel bundles + std::lock_guard KernelIDsGuard(m_KernelIDsMutex); + for (int I = 0; I < DeviceBinary->NumDeviceBinaries; I++) { sycl_device_binary RawImg = &(DeviceBinary->DeviceBinaries[I]); - // Acquire lock to read and modify maps for kernel bundles - std::lock_guard KernelIDsGuard(m_KernelIDsMutex); - auto DevImgIt = m_DeviceImages.find(RawImg); if (DevImgIt == m_DeviceImages.end()) continue; From 2097f66ebe2d2c82fb3ad3f6164ec4b621e46d8d Mon Sep 17 00:00:00 2001 From: Ian Li Date: Wed, 23 Apr 2025 15:08:34 -0400 Subject: [PATCH 5/6] Check worklist has tasks to do before locking mutex Co-authored-by: Steffen Larsen --- sycl/source/detail/program_manager/program_manager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index bfe430d00afde..8d1893b4a6bf2 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -756,7 +756,7 @@ ProgramManager::collectDependentDeviceImagesForVirtualFunctions( } } - { + if (!WorkList.empty()) { // Guard read access to m_VFSet2BinImage: // TODO: a better solution should be sought in the future, i.e. a different // mutex than m_KernelIDsMutex, check lock check pattern, etc. From 8ab33e0e4d1828d5654676958f0dbf6ce9d5ddf7 Mon Sep 17 00:00:00 2001 From: "Li, Ian" Date: Wed, 23 Apr 2025 12:15:41 -0700 Subject: [PATCH 6/6] Apply clang-format --- sycl/source/detail/program_manager/program_manager.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sycl/source/detail/program_manager/program_manager.cpp b/sycl/source/detail/program_manager/program_manager.cpp index 8d1893b4a6bf2..e7c2329136751 100644 --- a/sycl/source/detail/program_manager/program_manager.cpp +++ b/sycl/source/detail/program_manager/program_manager.cpp @@ -2153,7 +2153,8 @@ void ProgramManager::addImages(sycl_device_binaries DeviceBinary) { } void ProgramManager::removeImages(sycl_device_binaries DeviceBinary) { - if (DeviceBinary->NumDeviceBinaries == 0) return; + if (DeviceBinary->NumDeviceBinaries == 0) + return; // Acquire lock to read and modify maps for kernel bundles std::lock_guard KernelIDsGuard(m_KernelIDsMutex);