[SYCL] Add support for compressed BF16 device lib images in runtime #18108
Conversation
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| std::unique_ptr<char[]> Data(new char[ImgSize]); | ||
| std::memcpy(Data.get(), RawImg->BinaryStart, ImgSize); | ||
| auto DynBfloat16DeviceLibImg = | ||
| std::make_unique<DynRTDeviceBinaryImage>(std::move(Data), ImgSize); |
There was a problem hiding this comment.
Hi @jinge90 ,
I'm wondering why do we need to use dynamic RT device image and not a static one (RTDeviceBinaryImage)?
There was a problem hiding this comment.
Hi, @uditagarwal97
The reason is RTDeviceBinaryImage does not own the sycl_device_binary_struct, which itself does not own the raw binary data (e.g. SPIRV) at sycl_device_binary_struct::BinaryStart. The previous discussion is in: #17461 (comment)
Suppose following scenario, if main program dlopen 2 sycl dynamic library lib1 and lib2, both of them depend on bfloat16 devicelib. SYCL RT calls addImages for lib1 and keeps the required bfloat16 devicelib RTDeviceBinaryImages in program manager's m_Bfloat16DeviceLibImages for further dynamic symbol resolve. Then, SYCL RT loads lib2 and resolves dependent bfloat16 symbols with m_Bfloat16DeviceLibImages. The RTDeviceBinaryImage stored in m_Bfloat16DeviceLibImages is from lib1 and SYCL RT may dlclose lib1 when lib2 device code is still alive. To avoid this, we need to make sure bfloat16 devicelib image is alive during the lifetime of the sycl programs, so we use dynamic device binary image.
I know little about 'CompressedDeviceImage', I think we need to do same thing if the compressed device image may be removed by SYCL RT.
Adding @jopperm for some suggestion.
Thanks very much.
There was a problem hiding this comment.
I see, makes sense then. CompressedDeviceImage is same as DynRTDeviceImage, i.e., it also allocates data on heap but, the former can also handle compressed images.
There was a problem hiding this comment.
Hi, @uditagarwal97
Just double confirm, do you mean the CompressedDeviceImage holds the compressed device image which is located on head and it be alive during the whole lifetime of current sycl program? If so, the patch is OK to be merged.
Thanks very much.
There was a problem hiding this comment.
After a quick look, my understanding is that CompressedRTDeviceBinaryImage owns a copy of the original image's sycl_device_binary struct, and it owns the decompressed bytes, so that would be fine 👍
There was a problem hiding this comment.
Yes, just like DynRTDeviceImage, CompressedRTDeviceBinaryImage also owns the device image bytes.
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
uditagarwal97
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
uditagarwal97
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
Created a JIRA for unrelated, flaky Gen12 windows failure: #18142. Jenkins failure seems like an infrastructure issue. |
#16729 added support to embed BF16 device lib in executable using dynamic linking feature. However, it does not work with
--offload-compress. This PR fixes that.See CMPLRLLVM-66723