[SYCL] Delete symbol based info with the last image referencing it #19659

sergey-semenov · 2025-07-31T14:40:11Z

Prior to this patch, symbol based info (e.g. kernel id, kernel assert usage or images containing an exported symbol) was deleted whenever an image referencing it was removed. This is incorrect since multiple images can contain the same symbol.

While unlikely to cause any problems now (since those images usually all get removed with one call to removeImages after another), this will cause issues once kernel name based kernel caches start getting cleaned up in the same manner.

Prior to this patch, kernel name based info (e.g. kernel id or assert usage) was deleted whenever an image referencing it was removed. This is technically incorrect since multiple images can contain the same kernel name.

This reverts commit dcb4788.

KseniyaTikhomirova

LGTM

KseniyaTikhomirova · 2025-08-11T16:13:13Z

sycl/source/detail/program_manager/program_manager.cpp

@@ -2115,6 +2118,18 @@ void ProgramManager::addImages(sycl_device_binaries DeviceBinary) {
    addImage(&(DeviceBinary->DeviceBinaries[I]));
 }

+template <typename MultimapT, typename KeyT, typename ValT>
+void removeFromMultimap(MultimapT &Map, const KeyT &Key, const ValT &Val,


nit: I would change the name to removeFromMultimapByVal.

KseniyaTikhomirova · 2025-08-11T16:20:53Z

sycl/source/detail/program_manager/program_manager.cpp

-        m_VFSet2BinImage.erase(SetName);
+      for (const auto &SetName : detail::split_string(StrValue, ',')) {
+        auto It = m_VFSet2BinImage.find(SetName);
+        assert(It != m_VFSet2BinImage.end());


I'd suggest to create a tiny separate method for this.

I'm not sure about outlining, but some inline comment would help...

aelovikov-intel · 2025-08-11T16:31:32Z

sycl/source/detail/program_manager/program_manager.cpp

@@ -2140,44 +2155,67 @@ void ProgramManager::removeImages(sycl_device_binaries DeviceBinary) {
    // Unmap the unique kernel IDs for the offload entries
    for (sycl_offload_entry EntriesIt = EntriesB; EntriesIt != EntriesE;
         EntriesIt = EntriesIt->Increment()) {
-
+      const char *Name = EntriesIt->GetName();


Can it be std::string_view instead?

aelovikov-intel · 2025-08-11T16:37:17Z

sycl/source/detail/program_manager/program_manager.cpp

+      // Remove everything associated with this KernelName if this is the last
+      // image referencing it, otherwise remove just the ID -> Img mapping.
+      auto RefCountIt = m_KernelNameRefCount.find(Name);
+      assert(RefCountIt != m_KernelNameRefCount.end());
+      int &RefCount = RefCountIt->second;
+      assert(RefCount > 0);
+      --RefCount;

-      if (auto It = m_KernelName2KernelIDs.find(EntriesIt->GetName());
+      if (auto It = m_KernelName2KernelIDs.find(Name);
          It != m_KernelName2KernelIDs.end()) {
-        m_KernelIDs2BinImage.erase(It->second);
-        m_KernelName2KernelIDs.erase(It);
+        if (RefCount == 0) {
+          m_KernelIDs2BinImage.erase(It->second);
+          m_KernelName2KernelIDs.erase(It);
+        } else {
+          removeFromMultimap(m_KernelIDs2BinImage, It->second, Img);
+        }
+      }
+
+      if (RefCount == 0) {
+        m_KernelUsesAssert.erase(Name);
+        m_KernelImplicitLocalArgPos.erase(Name);
+        m_KernelNameRefCount.erase(RefCountIt);


This is ugly enough to justify an extended comment. I'm not familiar with the code but it's very unexpected to have several branches for if (RefCount == 0), it looks like some ownership is spread between several places which is never good. Can you explain what is going on here?

sergey-semenov had a problem deploying to WindowsCILock July 31, 2025 14:40 — with GitHub Actions Failure

sergey-semenov temporarily deployed to WindowsCILock July 31, 2025 15:09 — with GitHub Actions Inactive

aelovikov-intel self-requested a review August 1, 2025 16:22

sergey-semenov added 3 commits August 4, 2025 05:23

Merge branch 'sycl' into kernelnamecleanup

91cbc30

Improve m_KernelIDs2BinImage cleanup

0c50f13

Fix cleanup of exported symbol images map

338b1a4

sergey-semenov temporarily deployed to WindowsCILock August 4, 2025 14:10 — with GitHub Actions Inactive

sergey-semenov changed the title ~~[SYCL] Delete kernel name based info with the last image referencing it~~ [SYCL] Delete entry name based info with the last image referencing it Aug 4, 2025

sergey-semenov had a problem deploying to WindowsCILock August 4, 2025 14:45 — with GitHub Actions Failure

Check presence in ID->Img map instead of asserting it

2c2a073

sergey-semenov had a problem deploying to WindowsCILock August 4, 2025 15:38 — with GitHub Actions Failure

sergey-semenov had a problem deploying to WindowsCILock August 4, 2025 16:24 — with GitHub Actions Failure

Fix the if vs assert part

151b5ca

sergey-semenov temporarily deployed to WindowsCILock August 5, 2025 13:57 — with GitHub Actions Inactive

sergey-semenov temporarily deployed to WindowsCILock August 5, 2025 15:03 — with GitHub Actions Inactive

sergey-semenov added 2 commits August 5, 2025 16:42

Merge branch 'sycl' into kernelnamecleanup

623d661

Debug kernel_lin.cpp test

dcb4788

sergey-semenov had a problem deploying to WindowsCILock August 5, 2025 23:47 — with GitHub Actions Failure

sergey-semenov temporarily deployed to WindowsCILock August 6, 2025 00:15 — with GitHub Actions Inactive

sergey-semenov added 2 commits August 6, 2025 08:11

Revert "Debug kernel_lin.cpp test"

23166af

This reverts commit dcb4788.

Refcount cleanup

372a271

sergey-semenov temporarily deployed to WindowsCILock August 6, 2025 15:26 — with GitHub Actions Inactive

sergey-semenov had a problem deploying to WindowsCILock August 6, 2025 16:03 — with GitHub Actions Failure

sergey-semenov temporarily deployed to WindowsCILock August 6, 2025 16:03 — with GitHub Actions Inactive

sergey-semenov added 2 commits August 7, 2025 06:32

Extend cleanup unit tests

bd43afc

Add a unit test

b1f9365

sergey-semenov had a problem deploying to WindowsCILock August 8, 2025 16:22 — with GitHub Actions Error

sergey-semenov changed the title ~~[SYCL] Delete entry name based info with the last image referencing it~~ [SYCL] Delete symbol based info with the last image referencing it Aug 8, 2025

sergey-semenov marked this pull request as ready for review August 8, 2025 16:25

sergey-semenov requested a review from a team as a code owner August 8, 2025 16:25

sergey-semenov requested a review from KseniyaTikhomirova August 8, 2025 16:25

sergey-semenov had a problem deploying to WindowsCILock August 8, 2025 22:22 — with GitHub Actions Failure

Merge branch 'sycl' into kernelnamecleanup

286b1a7

sergey-semenov temporarily deployed to WindowsCILock August 11, 2025 10:48 — with GitHub Actions Inactive

sergey-semenov temporarily deployed to WindowsCILock August 11, 2025 11:15 — with GitHub Actions Inactive

KseniyaTikhomirova approved these changes Aug 11, 2025

View reviewed changes

aelovikov-intel reviewed Aug 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Delete symbol based info with the last image referencing it #19659

[SYCL] Delete symbol based info with the last image referencing it #19659

sergey-semenov commented Jul 31, 2025 •

edited

Loading

Uh oh!

KseniyaTikhomirova left a comment

Uh oh!

KseniyaTikhomirova Aug 11, 2025

Uh oh!

KseniyaTikhomirova Aug 11, 2025

Uh oh!

aelovikov-intel Aug 11, 2025

Uh oh!

aelovikov-intel Aug 11, 2025

Uh oh!

aelovikov-intel Aug 11, 2025

Uh oh!

Uh oh!

[SYCL] Delete symbol based info with the last image referencing it #19659

Are you sure you want to change the base?

[SYCL] Delete symbol based info with the last image referencing it #19659

Conversation

sergey-semenov commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KseniyaTikhomirova left a comment

Choose a reason for hiding this comment

Uh oh!

KseniyaTikhomirova Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

KseniyaTikhomirova Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Aug 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sergey-semenov commented Jul 31, 2025 •

edited

Loading