[SYCL] Keep multiple copies for bf16 device library image #17461

jinge90 · 2025-03-14T07:43:32Z

SYCL RT addImages function may be invoked multiple times for different sycl binary images, more than 1 of these sycl binary images may depend on bfloat16 device library. These bfloat16 device library images are provided by compiler and the implementation are stable now, so we only keep single copy for native and fallback version bfloat16 device library in program manager, these images will not be removed unless program manager is destroyed.

Signed-off-by: jinge90 <[email protected]>

…lose

Signed-off-by: jinge90 <[email protected]>

jopperm · 2025-03-18T23:12:49Z

sycl/source/detail/program_manager/program_manager.cpp

+    // For bfloat16 device library image, it doesn't include any kernel, device
+    // global, virtual function, so just skip adding it to any related maps.
+    // We only need to: 1). add exported symbols to m_ExportedSymbolImages. 2).
+    // add the device image to m_DeviceImages used for future clean up when
+    // removeImage is called. RefCount is used to keep how many user device
+    // images are depending on native/fallback bfloat16 device library image,
+    // the corresponding image will be added to m_ExportedSymbolImages and
+    // m_DeviceImages only when RefCount is 0. These RefCount are used when
+    // KernelIDsGuard is acquired by current thread.
+    {
+      auto Bfloat16DeviceLibProp = Img->getDeviceLibMetadata();
+      if (Bfloat16DeviceLibProp.isAvailable()) {
+        uint32_t IsNative =
+            DeviceBinaryProperty(*(Bfloat16DeviceLibProp.begin())).asUint32();
+        if (!m_Bfloat16DeviceLibRefCount[IsNative]) {
+          for (const sycl_device_binary_property &ESProp :
+               Img->getExportedSymbols()) {
+            m_ExportedSymbolImages.insert({ESProp->Name, Img.get()});
+          }
+          m_DeviceImages.insert({RawImg, std::move(Img)});
+        }
+        m_Bfloat16DeviceLibRefCount[IsNative] += 1;
+        continue;
+      }
+    }


Making the special handling explicit (and only doing the nececssary things) is a good idea 👍

jopperm · 2025-03-19T00:03:53Z

sycl/source/detail/program_manager/program_manager.cpp

+            m_ExportedSymbolImages.erase(ESProp->Name);
+          }
+
+          m_DeviceImages.erase(DevImgIt);


Not erasing the device image here when the refcount > 0 won't keep the underlying device binaries alive. Consider the following situation:

bundle A loaded, contributes bfloat dev lib into runtime -> refcount = 1

bundle B loaded, uses bfloat dev lib -> refcount = 2

bundle A is freed -> refcount = 1

bundle C loaded, uses bfloat dev lib -> refcount = 2, and crash when PM tries to link the kernels because m_ExportedSymbols points to image coming from bundle A, which has been destroyed

jopperm · 2025-03-19T00:21:37Z

As I commented inline, I don't think the refcounting alone fixes the underlying issue; rather the program manager would also need to take ownership of the bfloat device library image until the refcount drops to 0.

Given these complications, I think it would also be fair to just accept the presence of multiple copies of these special images. m_ExportedSymbols is already a multimap, so if we erase just the entries referencing the current image in removeImages (instead of blanket erase'ing of all entries with a given symbol name), we should be good.

jopperm · 2025-03-19T00:32:18Z

CC @steffenlarsen for potential relation to #17442.

…lose

jinge90 · 2025-03-19T07:16:04Z

As I commented inline, I don't think the refcounting alone fixes the underlying issue; rather the program manager would also need to take ownership of the bfloat device library image until the refcount drops to 0.

Given these complications, I think it would also be fair to just accept the presence of multiple copies of these special images. m_ExportedSymbols is already a multimap, so if we erase just the entries referencing the current image in removeImages (instead of blanket erase'ing of all entries with a given symbol name), we should be good.

Hi, @jopperm
I agree that keeping multiple copies in m_ExportedSymbols is acceptable. I worked out a way to move these special device images in a separate container and maintain it carefully to add/removeImages to guarantee only 1 copy exists but the code is too complicated and seems not to be the right direction.
The updated patch has following behavior:

Each time addImages is called to add a user sycl binary, the included bf16 devicelib image will be registered into m_ExportedSymbols map.
Each time removeImages is called to remove a user sycl binary, for any bf16 device image included, we check m_ExportedSymbols map and only remove the entries whose value is current bf16 device image ptr. When current sycl binary is removed, all entries from the removed sycl binary is cleared from m_ExportedSymbols map.

Thanks very much.

Signed-off-by: jinge90 <[email protected]>

jinge90 · 2025-04-01T09:34:30Z

Hi, @jopperm
Could you help review again?
Thanks very much.

jopperm

Minor feedback, approach LGTM. Please also add a description for the PR.

jopperm · 2025-04-01T07:37:54Z

sycl/source/detail/program_manager/program_manager.cpp

+  int Bfloat16DeviceLibVersion = -1;
+  if (m_Bfloat16DeviceLibImages[0].get() == BinImage)
+    Bfloat16DeviceLibVersion = 0;
+  if (m_Bfloat16DeviceLibImages[1].get() == BinImage)


Suggested change

if (m_Bfloat16DeviceLibImages[1].get() == BinImage)

else if (m_Bfloat16DeviceLibImages[1].get() == BinImage)

jopperm · 2025-04-01T07:41:23Z

sycl/source/detail/program_manager/program_manager.cpp

+      if (!LibVersion)
+        return true;
+
+      *LibVersion = 0;


Is it ok to use 0 (= a valid lib version) here? Should that be ~0U or 2 or so?

jopperm · 2025-04-01T10:44:30Z

sycl/test-e2e/KernelCompiler/sycl.cpp

+         test_device_libraries(q) || test_device_libraries(q) ||
+         test_device_libraries(q) || test_unsupported_options(q) ||


Missing test_esimd here (also, 3x test_device_libraries).

Fixed.
Thanks very much.

jopperm · 2025-04-01T10:56:23Z

sycl/source/detail/program_manager/program_manager.hpp

+  // and 1 for native version. These bfloat16 device library images are
+  // provided by compiler long time ago, we expect no further update, so
+  // keeping 1 copy should be OK.
+  std::unordered_map<uint32_t, DynRTDeviceBinaryImageUPtr>


Could this be just a std::array?

Hi, @jopperm
Yes, already updated the PR.
Thanks very much.

Signed-off-by: jinge90 <[email protected]>

jopperm

Thanks for iterating this, LGTM now. Please update the PR title and add a description.

…lose

SYCL RT addImages function may be invoked multiple times for different sycl binary images, more than 1 of these sycl binary images may depend on bfloat16 device library. These bfloat16 device library images are provided by compiler and the implementation are stable now, so we only keep single copy for native and fallback version bfloat16 device library in program manager, these images will not be removed unless program manager is destroyed. --------- Signed-off-by: jinge90 <[email protected]>

jinge90 requested a review from a team as a code owner March 14, 2025 07:43

jinge90 requested a review from againull March 14, 2025 07:43

jinge90 marked this pull request as draft March 14, 2025 07:43

jinge90 temporarily deployed to WindowsCILock March 14, 2025 07:43 — with GitHub Actions Inactive

jinge90 mentioned this pull request Mar 14, 2025

[SYCL][RTC] Adopt recent changes from sycl-post-link #17447

Merged

Skip unnecessary init for bf16 devicelib image

4ee0932

Signed-off-by: jinge90 <[email protected]>

jinge90 temporarily deployed to WindowsCILock March 14, 2025 08:20 — with GitHub Actions Inactive

jinge90 temporarily deployed to WindowsCILock March 14, 2025 09:55 — with GitHub Actions Inactive

skip bf16 devicelib image when merging metadata

57b5523

Signed-off-by: jinge90 <[email protected]>

jinge90 temporarily deployed to WindowsCILock March 14, 2025 10:29 — with GitHub Actions Inactive

jinge90 added 2 commits March 14, 2025 19:01

Merge remote-tracking branch 'upstream/sycl' into bf16_conversion_dlc…

57d14fd

…lose

Merge remote-tracking branch 'upstream/sycl' into bf16_conversion_dlc…

26f8895

…lose

jinge90 temporarily deployed to WindowsCILock March 18, 2025 09:05 — with GitHub Actions Inactive

jinge90 requested a review from jopperm March 18, 2025 09:06

Use Refcount to guard bf16 devicelib

34f396f

Signed-off-by: jinge90 <[email protected]>

jinge90 temporarily deployed to WindowsCILock March 18, 2025 09:39 — with GitHub Actions Inactive

jopperm reviewed Mar 19, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/sycl' into bf16_conversion_dlc…

b09663d

…lose

jinge90 temporarily deployed to WindowsCILock March 19, 2025 07:10 — with GitHub Actions Inactive

jinge90 requested a review from steffenlarsen March 19, 2025 07:34

allow multiple device image for bf16 devicelib symbols

fcbbe1c

Signed-off-by: jinge90 <[email protected]>

jinge90 had a problem deploying to WindowsCILock March 19, 2025 07:40 — with GitHub Actions Error

jinge90 temporarily deployed to WindowsCILock March 19, 2025 07:45 — with GitHub Actions Inactive

jinge90 temporarily deployed to WindowsCILock March 28, 2025 06:41 — with GitHub Actions Inactive

Create DynRTDeviceBinaryImage for bf16 devicelib

1351401

Signed-off-by: jinge90 <[email protected]>

jinge90 temporarily deployed to WindowsCILock March 28, 2025 07:11 — with GitHub Actions Inactive

jinge90 temporarily deployed to WindowsCILock March 28, 2025 10:15 — with GitHub Actions Inactive

fix clang format

640f679

Signed-off-by: jinge90 <[email protected]>

jinge90 temporarily deployed to WindowsCILock March 28, 2025 10:49 — with GitHub Actions Inactive

steffenlarsen requested a review from jopperm April 1, 2025 10:38

jopperm reviewed Apr 1, 2025

View reviewed changes

jinge90 temporarily deployed to WindowsCILock April 2, 2025 03:11 — with GitHub Actions Inactive

jinge90 marked this pull request as ready for review April 2, 2025 03:12

Fix sycl.cpp and use std::array to replace map for bf16 device images

24fa45a

Signed-off-by: jinge90 <[email protected]>

jinge90 had a problem deploying to WindowsCILock April 2, 2025 03:46 — with GitHub Actions Error

jinge90 temporarily deployed to WindowsCILock April 2, 2025 03:57 — with GitHub Actions Inactive

change default libversion to 2

472d1be

Signed-off-by: jinge90 <[email protected]>

jinge90 temporarily deployed to WindowsCILock April 2, 2025 04:33 — with GitHub Actions Inactive

jopperm approved these changes Apr 2, 2025

View reviewed changes

steffenlarsen mentioned this pull request Apr 2, 2025

[SYCL] Move kernel_compiler related information #17380

Merged

jinge90 temporarily deployed to WindowsCILock April 3, 2025 01:18 — with GitHub Actions Inactive

Merge remote-tracking branch 'upstream/sycl' into bf16_conversion_dlc…

a33f24d

…lose

jinge90 temporarily deployed to WindowsCILock April 3, 2025 01:53 — with GitHub Actions Inactive

steffenlarsen merged commit 80fd665 into intel:sycl Apr 3, 2025
34 of 35 checks passed

This was referenced Apr 21, 2025

[SYCL] Test bfloat16 devicelib with dlopen/dlclose #18106

Merged

[SYCL] Add support for compressed BF16 device lib images in runtime #18108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Keep multiple copies for bf16 device library image #17461

[SYCL] Keep multiple copies for bf16 device library image #17461

jinge90 commented Mar 14, 2025 •

edited

Loading

jopperm Mar 18, 2025

jopperm Mar 19, 2025

jopperm commented Mar 19, 2025 •

edited

Loading

jopperm commented Mar 19, 2025

jinge90 commented Mar 19, 2025 •

edited

Loading

jinge90 commented Apr 1, 2025

jopperm left a comment

jopperm Apr 1, 2025

jopperm Apr 1, 2025

jopperm Apr 1, 2025

jinge90 Apr 2, 2025

jopperm Apr 1, 2025

jinge90 Apr 2, 2025

jopperm left a comment

	if (m_Bfloat16DeviceLibImages[1].get() == BinImage)
	else if (m_Bfloat16DeviceLibImages[1].get() == BinImage)

		test_device_libraries(q) \|\| test_device_libraries(q) \|\|
		test_device_libraries(q) \|\| test_unsupported_options(q) \|\|

[SYCL] Keep multiple copies for bf16 device library image #17461

[SYCL] Keep multiple copies for bf16 device library image #17461

Conversation

jinge90 commented Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jopperm commented Mar 19, 2025 • edited Loading

jopperm commented Mar 19, 2025

jinge90 commented Mar 19, 2025 • edited Loading

jinge90 commented Apr 1, 2025

jopperm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jopperm left a comment

Choose a reason for hiding this comment

jinge90 commented Mar 14, 2025 •

edited

Loading

jopperm commented Mar 19, 2025 •

edited

Loading

jinge90 commented Mar 19, 2025 •

edited

Loading