[SYCL] Support --offload-compress in clang-linker-wrapper by bviyer · Pull Request #22605 · intel/llvm

bviyer · 2026-07-10T23:10:58Z

Forward --offload-compress (and --offload-compression-level=) from the driver to clang-linker-wrapper under the new offloading driver. In wrapSYCLBinariesFromFile, zstd-compress each SYCL device image whose payload is at or above --offload-compression-threshold= (default 512 bytes) and tag it with the new BIF_Compressed image format so the SYCL runtime decompresses lazily via CompressedRTDeviceBinaryImage.

Significant help from Claude Opus 4.7.

sarnex · 2026-07-14T18:17:12Z

+// The --compress flag above is a different mechanism: for HIP only, it is
+// forwarded verbatim to a clang-offload-bundler subprocess which does the
+// compression itself.
+def offload_compress : Flag<["--"], "offload-compress">,


Instead of adding new flags could we just re-use the existing ones? Then I don't think we need any driver changes, it seems addOffloadCompressArgs should do what we need if we make clang-linker-wrapper do the right thing

Fixed. I made it use --compress and --compress-level=

sarnex · 2026-07-14T18:18:05Z

+  BIF_Native,        // Native Image kind
+  BIF_SPIRV,         // SPIR-V
+  BIF_LLVMBC,        // LLVM bitcode
+  BIF_CompressedNone // zstd-compressed image; format-of-original unknown


Maybe we could just call it BIF_Compressed unless we need different BIFCompressed versions for different original formats, which it seems like we don't

sarnex · 2026-07-14T18:18:43Z

  case SYCLBinaryImageFormat::BIF_LLVMBC:
    return 3;
+  case SYCLBinaryImageFormat::BIF_CompressedNone:
+    return 4;


I assume the SYCL runtime already knows how to handle this and the format matches the old offload model?

sarnex · 2026-07-14T18:20:56Z

+          "'--offload-compress' is specified but zstd is not available");
+
+    int Level = 10;      // default zstd level, matches clang-offload-wrapper
+    int Threshold = 512; // skip compression below this many bytes


I wonder if we can h ave a shared function that both clang-offload-wrapper and clang-linker-wrapper use until we remove old offload model support. Maybe we could put it in llvm/lib/Frontend/Offloading/Utility.cpp?

This is fixed here ec31530. Its a bit messy.

bader · 2026-07-15T23:35:51Z

@bviyer, please, remove .gdbinit from PR.

bviyer · 2026-07-15T23:41:32Z

@bviyer, please, remove .gdbinit from PR.

Sorry. Fixed.

sarnex

nice cleanup! some comments below

sarnex · 2026-07-16T15:31:38Z

+// CHECK-NO-COMPRESS-NOT: {{.*}}clang-linker-wrapper{{.*}}"--compress"
+
+// --no-offload-compress overrides an earlier --offload-compress.
+// RUN: %clangxx -### -fsycl --offload-new-driver --offload-compress \


should we check the opposite case, where we have --no-offload-compress --offload-compress

sarnex · 2026-07-16T15:32:37Z

+  // consume them here and zstd-compress each image payload in place, flipping
+  // its Format to BIF_Compressed. The SYCL runtime recognizes that tag and
+  // decompresses lazily via CompressedRTDeviceBinaryImage.
+  if (Args.hasArg(OPT_compress)) {


could we move this all into a separate function like compressSYCLImages or something?

sarnex · 2026-07-16T15:36:12Z

+// Forward-declared instead of #include "llvm/Frontend/Offloading/Utility.h"
+// because pulling in that header (transitively llvm/Object/OffloadBinary.h)
+// makes llvm::object::OffloadKind visible via `using namespace llvm::object`
+// above, which collides with the local anonymous-namespace `enum OffloadKind`


IMO instead of this workaround we should fix the local anonymous namespace issue, maybe rename it to something that shows how it's different from the OffloadBinary.h one if really we need a separate enum

Renamed to OffloadModelKind (didn't want to remove the kind since it was used in several places and looks like it means something).

sarnex · 2026-07-16T15:38:42Z

+        // pre-refactor behavior which errored regardless of image kind or
+        // format. The guarded branch below would otherwise skip the check for
+        // non-SYCL / explicit-format images.
+        if (OffloadCompressDevImgs && !llvm::compression::zstd::isAvailable())


IMO changing the error behavior is fine, if we don't actually try to compress anything it's fine to not error, having this extra check is confusing

sarnex · 2026-07-16T15:45:31Z

+
+    for (auto &Image : Images) {
+      SmallVector<uint8_t, 0> CompressedBytes;
+      Expected<bool> Compressed = offloading::compressSYCLDeviceImage(


can we make it a bit clearer on what the return type is with something like

Expected<bool> DidCompressOrErr = ... if (!DidCompressOrErr) return DidCompressOrErr.takeError(); bool DidCompres = *DidCompressOrErr; /// use DidCompress

At first it was unclear to me what the bool represented so I think making it explicit would help readability

Fixed. However, I didn't create a new variable called DidCompress I just used the check on (*DidCompressOrErr). Generally, I do not create new variables unless the item is used in multiple places.

sarnex · 2026-07-16T15:47:43Z

i think we lost the LLVM_ENABLE_EXCEPTIONS handling which we probably need

sarnex

did you forget to push your changes to the PR? i'm not seeing any new commits with fixes

sarnex

lgtm thanks! just some final nits

sarnex · 2026-07-16T19:18:13Z

+/// clang-offload-wrapper also uses; the surrounding loop isn't shared
+/// because that tool operates on one image at a time and emits an LLVM
+/// Constant directly, not a SYCLImage list.
+static Error compressSYCLImages(SmallVectorImpl<offloading::SYCLImage> &Images,


sorry instead can we put this in the namespace sycl starting at line ~515 and call it compressImages? Thanks

sarnex · 2026-07-16T19:18:55Z

+/// tagging its Format as BIF_Compressed. --compress and --compression-level=
+/// are the same flags HIP forwards to clang-offload-bundler; for SYCL we
+/// consume them here. The per-image work delegates to
+/// offloading::compressSYCLDeviceImage, which the old-driver


im not sure we need to mention the old offloading model here or anywhere in code comments in new offload model code

sarnex · 2026-07-16T19:19:57Z

-// enum.
-enum OffloadKind {
+// enum. Distinct from llvm::object::OffloadKind (in Object/OffloadBinary.h),
+// which is the 16-bit bitmask on the OffloadBinary wire format


im not sure what wire format means

Yep, it was an artifact of something I was typing. I reworded to "...in the OffloadBinary header."

sarnex · 2026-07-16T19:20:39Z

+// command-line-parsed tag with an extra Host slot and First/Last sentinels,
+// serialized as a uint8_t into the SYCL runtime's sycl_device_binary_struct
+// Kind field.
+enum OffloadModelKind {


maybe OldOffloadKind?

Forward --offload-compress (and --offload-compression-level=) from the driver to clang-linker-wrapper under the new offloading driver. In wrapSYCLBinariesFromFile, zstd-compress each SYCL device image whose payload is at or above --offload-compression-threshold= (default 512 bytes) and tag it with the new BIF_CompressedNone image format so the SYCL runtime decompresses lazily via CompressedRTDeviceBinaryImage.

ClangOffLoadWrappper and ClangLinkerWrapper.

YuriPlyakhin

Please update PR description: BIF_CompressedNone -> BIF_Compressed

YuriPlyakhin

Could you please add a functional test for the new compression path to test changes in clang-linker-wrapper.cpp?
Test would run clang-linker-wrapper --compress and would verify the emitted image is tagged with BIF_Compressed.
Also it would check invalid value for --offload-compression-level= error message.
FYI: The old wrapper is tested by clang-offload-wrapper-zstd.c

Co-authored-by: Yury Plyakhin <yury.plyakhin@intel.com>

bviyer · 2026-07-17T17:36:12Z

Please update PR description: BIF_CompressedNone -> BIF_Compressed

Fixed.

path in ClangLinkerWrapper.

bviyer · 2026-07-17T17:52:06Z

Could you please add a functional test for the new compression path to test changes in clang-linker-wrapper.cpp? Test would run clang-linker-wrapper --compress and would verify the emitted image is tagged with BIF_Compressed. Also it would check invalid value for --offload-compression-level= error message. FYI: The old wrapper is tested by clang-offload-wrapper-zstd.c

Added test here: 48bf119

sarnex · 2026-07-17T17:59:09Z

+
+// RUN: %clang -cc1 -fsycl-is-device -disable-llvm-passes -triple=spir64-unknown-unknown %s -emit-llvm-bc -o %t.device.bc
+// RUN: llvm-offload-binary -o %t.fat --image=file=%t.device.bc,kind=sycl,triple=spir64-unknown-unknown
+// RUN: %clang -cc1 %s -triple=x86_64-unknown-linux-gnu -emit-obj -o %t.o -fembed-offload-object=%t.fat


i think we also need x86-registered-target in REQUIRES

sarnex · 2026-07-17T18:01:46Z

+// CHECK-COMPRESS: [Compression] Original image size:
+// CHECK-COMPRESS: [Compression] Compressed image size:


i think we should be able to do basic math operations in Filecheck https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-numeric-substitution-blocks

could we at least verify the compressed size is smaller than the original?

sarnex

thanks!

sys-ce-bb · 2026-07-17T22:16:42Z

@intel/llvm-gatekeepers please consider merging

YuriPlyakhin · 2026-07-18T03:03:32Z

+// --- Prepare test data: build a bitcode module, pack it into an
+// llvm-offload-binary, and embed it into an ELF object as the linker-wrapper
+// input. The bitcode is large enough (well over the 512-byte compression
+// threshold in wrapSYCLBinariesFromFile) that compression actually runs.
+


nit. I think comments are unnecessarily verbose.

Suggested change

// --- Prepare test data: build a bitcode module, pack it into an

// llvm-offload-binary, and embed it into an ELF object as the linker-wrapper

// input. The bitcode is large enough (well over the 512-byte compression

// threshold in wrapSYCLBinariesFromFile) that compression actually runs.

// Prepare test data. The bitcode is over the 512-byte compression

// threshold, so the compression actually runs.

YuriPlyakhin · 2026-07-18T03:10:05Z

+// Assert that the compressed size is strictly smaller than the original size
+// (i.e. ORIG - COMP > 0). FileCheck's numeric matching form only supports the
+// `==` constraint — there is no `<` or `>` — but sub() rejects underflow at
+// expression-evaluation time. We attach sub(ORIG, COMP) to a CHECK-NOT
+// pattern that begins with a sentinel string never present in the output:
+//   * when ORIG > COMP, sub() succeeds; the substituted pattern is
+//     "COMPRESSION_SIZE_CHECK<some-number>", which doesn't appear, so the
+//     CHECK-NOT is satisfied vacuously.
+//   * when COMP >= ORIG, sub() underflows and FileCheck fails the pattern.


nit. too verbose for me :)

Suggested change

// Assert that the compressed size is strictly smaller than the original size

// (i.e. ORIG - COMP > 0). FileCheck's numeric matching form only supports the

// `==` constraint — there is no `<` or `>` — but sub() rejects underflow at

// expression-evaluation time. We attach sub(ORIG, COMP) to a CHECK-NOT

// pattern that begins with a sentinel string never present in the output:

// * when ORIG > COMP, sub() succeeds; the substituted pattern is

// "COMPRESSION_SIZE_CHECK<some-number>", which doesn't appear, so the

// CHECK-NOT is satisfied vacuously.

// * when COMP >= ORIG, sub() underflows and FileCheck fails the pattern.

// Assert ORIG - COMP > 0.

// FileCheck's numeric matching form only supports the

// `==` constraint, but sub() rejects underflow at

// expression-evaluation time. We attach sub(ORIG, COMP) to a CHECK-NOT

// pattern that begins with a sentinel string never present in the output:

// * when ORIG > COMP, sub() succeeds; the substituted pattern is

// "COMPRESSION_SIZE_CHECK<some-number>", which doesn't appear, so the

// CHECK-NOT is satisfied.

// * when COMP >= ORIG, sub() underflows and FileCheck fails the pattern.

YuriPlyakhin · 2026-07-18T03:13:30Z

+// The tgt_device_image struct is { i16 Version, i8 Kind, i8 Format, ptr ... };
+// Kind for SYCL is 4, Format for BIF_Compressed is also 4.


nit. All we need is to check BIF_Compressed and it is already explained in the comments above that it is 4. The check is fine, but I don't think we need to provide verbose comments explaining each small detail not related to the functionality we test.

Suggested change

// The tgt_device_image struct is { i16 Version, i8 Kind, i8 Format, ptr ... };

// Kind for SYCL is 4, Format for BIF_Compressed is also 4.

YuriPlyakhin

LGTM. Few nits - all related to too verbose comments. Feel free to ignore. Thanks! :)

bviyer requested review from YuriPlyakhin and maksimsab July 12, 2026 15:23

bviyer marked this pull request as ready for review July 14, 2026 14:05

bviyer requested review from a team as code owners July 14, 2026 14:05

sarnex reviewed Jul 14, 2026

View reviewed changes

bviyer requested a review from bader as a code owner July 15, 2026 23:27

bviyer force-pushed the bviyer-sycl-compress-support branch from 2361b30 to ec31530 Compare July 15, 2026 23:30

bviyer requested a review from sarnex July 15, 2026 23:32

bader removed their request for review July 15, 2026 23:52

sarnex reviewed Jul 16, 2026

View reviewed changes

bviyer requested a review from sarnex July 16, 2026 19:16

sarnex approved these changes Jul 16, 2026

View reviewed changes

bviyer added 9 commits July 16, 2026 14:36

Fixed a mistaken test

4ad4d5f

Added two of the three changes requested by Nick.

ba1bedf

Moved some functionality to a common function to share between

b3162a6

ClangOffLoadWrappper and ClangLinkerWrapper.

Removed .gdbinit

84e14dd

Fixed changes mentioned by Nick

22bb108

Fixed the test

5b71eed

Fixed more issues mentioned by Nick.

8fb71fc

Added changes after rebase

e79b396

bviyer force-pushed the bviyer-sycl-compress-support branch from e45280d to e79b396 Compare July 16, 2026 22:19

mdtoguchi approved these changes Jul 16, 2026

View reviewed changes

YuriPlyakhin reviewed Jul 17, 2026

View reviewed changes

Comment thread llvm/include/llvm/Frontend/Offloading/Utility.h Outdated

YuriPlyakhin reviewed Jul 17, 2026

View reviewed changes

Comment thread clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp Outdated

bviyer and others added 2 commits July 17, 2026 12:34

Update llvm/include/llvm/Frontend/Offloading/Utility.h

e4a4991

Co-authored-by: Yury Plyakhin <yury.plyakhin@intel.com>

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

537acd9

Co-authored-by: Yury Plyakhin <yury.plyakhin@intel.com>

Added a functional test for the new compression

48bf119

path in ClangLinkerWrapper.

bviyer requested a review from YuriPlyakhin July 17, 2026 17:56

Fixed a clang-formatting issue

a609844

sarnex reviewed Jul 17, 2026

View reviewed changes

Added check that compressed size is less than original size

8d91041

bviyer requested a review from sarnex July 17, 2026 19:59

sarnex approved these changes Jul 17, 2026

View reviewed changes

YuriPlyakhin reviewed Jul 18, 2026

View reviewed changes

YuriPlyakhin approved these changes Jul 18, 2026

View reviewed changes

		// CHECK-COMPRESS: [Compression] Original image size:
		// CHECK-COMPRESS: [Compression] Compressed image size:

		// The tgt_device_image struct is { i16 Version, i8 Kind, i8 Format, ptr ... };
		// Kind for SYCL is 4, Format for BIF_Compressed is also 4.

Uh oh!

Conversation

bviyer commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader commented Jul 15, 2026

Uh oh!

bviyer commented Jul 15, 2026

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bviyer Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YuriPlyakhin left a comment

Choose a reason for hiding this comment

Uh oh!

YuriPlyakhin left a comment

Choose a reason for hiding this comment

bviyer commented Jul 10, 2026 •

edited

Loading

bviyer Jul 16, 2026 •

edited

Loading

YuriPlyakhin Jul 18, 2026 •

edited

Loading

YuriPlyakhin Jul 18, 2026 •

edited

Loading

YuriPlyakhin Jul 18, 2026 •

edited

Loading