[SYCL][Offload] Add SYCLBIN format and dump tool #16873

steffenlarsen · 2025-02-03T16:03:09Z

This commit adds a new option --syclbin to the clang-linker-wrapper tool to produce SYCLBIN files from packaged device code binaries. The SYCLBIN format parsing added with these changes follow the format defined in the SYCLBIN design document.

To help both users and the testing framework, a tool (syclbin-dump) for printing information about a SYCLBIN file is added.

This commit adds a new option `--syclbin` to the clang-linker-wrapper tool to produce SYCLBIN files from packaged device code binaries. The SYCLBIN format parsing added with these changes follow the format defined in the SYCLBIN design document. To help both users and the testing framework, a tool (syclbin-dump) for printing information about a SYCLBIN file is added. Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen · 2025-02-03T16:03:20Z

Design document: #16872

sarnex · 2025-02-03T16:05:22Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

@@ -37,6 +37,7 @@
 #include "llvm/Object/IRObjectFile.h"


hopefully we're planning on upstreaming this?

Absolutely! I am told there are some refactorings coming in for ClangLinkerWrapper, so I wanted to get some pieces in for now so development of other parts, like the driver, can go ahead in parallel.

sarnex

just a quick first pass

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

sarnex · 2025-02-03T16:13:55Z

llvm/include/llvm/SYCLLowerIR/ModuleSplitter.h

@@ -18,6 +18,7 @@
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/Function.h"
+#include "llvm/IR/Module.h"


nit: do we need this instead of forward declaring?

We do. Unlike shared_ptr, unique_ptr insists on the contained type not being incomplete.

But why do we need this in a header file which is otherwise unchanged? It seems like either this #include should be moved straight into .cpp file, or into another .h file

It seems to be a dependency bug that just wasn't noticed because Module.h was included before ModuleSplitter.h in all other cases.

sycl/test/syclbin/simple_kernel.cpp

sycl/tools/syclbin-dump/syclbin-dump.cpp

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

sarnex · 2025-02-03T16:19:45Z

llvm/lib/Object/SYCLBIN.cpp

+namespace {
+
+template <typename T> void BinaryWriteInteger(raw_ostream &OS, T Val) {
+  static_assert(std::is_integral_v<T>);


nit: we could consider using SFINAE instead of the assert, but maybe this way the error is more clear

Since the name is explicit about it needing to be an integer, I like to think the error from a static assertion is clearer. SFINAE would've been nicer if we wanted it to select between implementations.

Do we really care about T being an integer? We do reinterpret cast and write raw bytes, the source types shouldn't matter at all, right?

It may be a bit of a minor optimization, but this function passes Val by value, which for an arbitrary T may be expensive. For integers it may be faster than passing by reference and should never be any more expensive, so that is the reason for the restriction.

AlexeySachkov · 2025-02-03T18:55:50Z

llvm/include/llvm/Object/SYCLBIN.h

+
+namespace object {
+
+class SYCLBIN : public Binary {


The main question I have here is why don't we re-use existing OffloadBinary subclass?

llvm/llvm/include/llvm/Object/OffloadBinary.h

Line 62 in 8461560

class OffloadBinary : public Binary {

If we need a hierarchical structure, then we just make it nested by putting one OffloadBinary into each other, or concatenating a few of them together to represent a list.
We can subclass if we want to provide some custom getters like getArch.

OpenMP offloading emits this data structure and libompoffload (that is going to be turned into liboffload and eventually is what we will use instead of UR once we are in upstream) expects OffloadBinary.

I also think that whatever we produce for regular SYCL app compilation, for online compilation from SYCL sources and for SYCLBIN should have the same format for simplicity and uniformity of handling it in SYCL RT.
From that point of view, I vote for re-using OffloadBinary even more, because whilst I can at least somehow justify a custom format for SYCLBIN, I can't justify us not using OffloadBinary for regular SYCL compilation flow because it provides everything we have in our existing legacy custom format.

I agree, we should eventually switch even the common structure of kernel code over to this new format.

However, I think making this a subclass of OffloadBinary makes the abstraction a little less obvious. The current structure is that the SYCLBIN binary is contained inside the image of an offload binary, so they are not related formats but rather nested formats.

We can subclass if we want to provide some custom getters like getArch.

SYCLBIN does not have architecture at the top-level, as different contained modules can target different device architectures, so I worry that merging the two would make getters like it confusing.

The current structure is that the SYCLBIN binary is contained inside the image of an offload binary, so they are not related formats but rather nested formats.

Oh, so the idea is to put SYCLBIN into OffloadBinary::OffloadingImage::Image, right? If so, then I withdraw my objections. In that case we can still have single top-level structure as an interface with SYCLBIN being a specialized version of the underlying content.

We should add a comment to this class declaration to describe that it is supposed to be used as a more complicated from of OffloadBinary::OffloadingImage::Image to simplify working with it.

I've added a comment and removed its inheritance from Binary.

Signed-off-by: Larsen, Steffen <[email protected]>

AlexeySachkov

Haven't yet looked at clang-linker-wrapper and SYCLBIN::read/write implementations

llvm/include/llvm/Object/SYCLBIN.h

AlexeySachkov · 2025-02-04T12:41:44Z

llvm/include/llvm/Object/SYCLBIN.h

+  SmallVector<AbstractModule, 4> AbstractModules;
+
+private:
+  SYCLBIN(const SYCLBIN &Other) = delete;


Why do you need it to be declared as private to delete it? If we explicitly deleting it, then I think it is better to put it next to other constructors so that they are all next to each other.

AlexeySachkov · 2025-02-04T12:48:28Z

llvm/include/llvm/Object/SYCLBIN.h

+
+namespace object {
+
+class SYCLBIN : public Binary {


We should add a comment to this class declaration to describe that it is supposed to be used as a more complicated from of OffloadBinary::OffloadingImage::Image to simplify working with it.

llvm/include/llvm/Object/Binary.h

llvm/include/llvm/Object/SYCLBIN.h

llvm/lib/Object/SYCLBIN.cpp

AlexeySachkov · 2025-02-04T13:20:20Z

llvm/lib/Object/SYCLBIN.cpp

+    for (const ModuleDesc &Desc : ModuleDescs) {
+      for (const module_split::SplitModule &SM : Desc.SplitModules) {


This is a very long for loop, I suggest that we outline some elements of it into smaller helper functions so that it is easier to grasp high-level logic and keep track of indentation levels

AlexeySachkov · 2025-02-04T13:26:58Z

llvm/include/llvm/Object/SYCLBIN.h

+  static Expected<SmallString<0>> write(const SmallVector<ModuleDesc> &);
+
+  static Expected<std::unique_ptr<SYCLBIN>> read(MemoryBufferRef Source);


Implementation of read/write is quite complex, it has to be exhaustively tested. I'm not saying that all the testing should be here from day 1, but we at least need to lay some groundwork for it. For example what you can do is to have a unit-test which does write, then read and checks that result is the same as the initial input. See e471ba3 for inspiration.

sycl/tools/syclbin-dump/syclbin-dump.cpp

sycl/tools/syclbin-dump/CMakeLists.txt

Signed-off-by: Larsen, Steffen <[email protected]>

…_wrapper_impl

steffenlarsen requested a review from asudarsa February 3, 2025 16:03

steffenlarsen requested review from a team as code owners February 3, 2025 16:03

steffenlarsen requested a review from KseniyaTikhomirova February 3, 2025 16:03

steffenlarsen had a problem deploying to WindowsCILock February 3, 2025 16:04 — with GitHub Actions Failure

sarnex reviewed Feb 3, 2025

View reviewed changes

AlexeySachkov reviewed Feb 3, 2025

View reviewed changes

steffenlarsen added 3 commits February 4, 2025 00:46

Fix include mistake

43c6227

Signed-off-by: Larsen, Steffen <[email protected]>

Address comments

86e3128

Signed-off-by: Larsen, Steffen <[email protected]>

SPIRV -> SPIR-V

1685f0f

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen had a problem deploying to WindowsCILock February 4, 2025 08:56 — with GitHub Actions Failure

steffenlarsen temporarily deployed to WindowsCILock February 4, 2025 09:51 — with GitHub Actions Inactive

Ignore unused variable in syclbin-dump

a024f11

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen had a problem deploying to WindowsCILock February 4, 2025 10:34 — with GitHub Actions Error

Disable on Windows until driver option is in place

716bdab

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen had a problem deploying to WindowsCILock February 4, 2025 11:13 — with GitHub Actions Error

Fix fault revert

d07b06c

Signed-off-by: Larsen, Steffen <[email protected]>

steffenlarsen temporarily deployed to WindowsCILock February 4, 2025 11:53 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to WindowsCILock February 4, 2025 12:40 — with GitHub Actions Inactive

AlexeySachkov reviewed Feb 4, 2025

View reviewed changes

AlexeySachkov mentioned this pull request Feb 4, 2025

[SYCL][Docs] Add SYCLBIN feature and format design document #16872

Open

KseniyaTikhomirova reviewed Feb 5, 2025

View reviewed changes

sycl/tools/syclbin-dump/CMakeLists.txt Outdated Show resolved Hide resolved

steffenlarsen added 2 commits February 6, 2025 02:32

Address comments and improve code

b598b66

Signed-off-by: Larsen, Steffen <[email protected]>

Merge remote-tracking branch 'intel/sycl' into steffen/syclbin_linker…

a4324ab

…_wrapper_impl

steffenlarsen temporarily deployed to WindowsCILock February 6, 2025 10:37 — with GitHub Actions Inactive

steffenlarsen temporarily deployed to WindowsCILock February 6, 2025 11:27 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][Offload] Add SYCLBIN format and dump tool #16873

[SYCL][Offload] Add SYCLBIN format and dump tool #16873

steffenlarsen commented Feb 3, 2025

steffenlarsen commented Feb 3, 2025

sarnex Feb 3, 2025

steffenlarsen Feb 3, 2025

sarnex left a comment

sarnex Feb 3, 2025

steffenlarsen Feb 4, 2025

AlexeySachkov Feb 4, 2025

steffenlarsen Feb 5, 2025

sarnex Feb 3, 2025

steffenlarsen Feb 4, 2025

AlexeySachkov Feb 4, 2025

steffenlarsen Feb 5, 2025

AlexeySachkov Feb 3, 2025

steffenlarsen Feb 4, 2025

AlexeySachkov Feb 4, 2025

AlexeySachkov Feb 4, 2025

steffenlarsen Feb 6, 2025

AlexeySachkov left a comment

AlexeySachkov Feb 4, 2025

AlexeySachkov Feb 4, 2025

AlexeySachkov Feb 4, 2025

AlexeySachkov Feb 4, 2025

		for (const ModuleDesc &Desc : ModuleDescs) {
		for (const module_split::SplitModule &SM : Desc.SplitModules) {

		static Expected<SmallString<0>> write(const SmallVector<ModuleDesc> &);

		static Expected<std::unique_ptr<SYCLBIN>> read(MemoryBufferRef Source);

[SYCL][Offload] Add SYCLBIN format and dump tool #16873

Are you sure you want to change the base?

[SYCL][Offload] Add SYCLBIN format and dump tool #16873

Conversation

steffenlarsen commented Feb 3, 2025

steffenlarsen commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarnex 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

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

AlexeySachkov 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