[NewOffloadModel] Fix transition of compile options from image to SYCL Offload Wrapper #19579

maksimsab · 2025-07-24T14:32:12Z

This patch fixes a handling of compile/link options encoded in images.
Also the patch fixes a redirection to backend compiler and offload
wrapper depending on the Triple (AOT or JIT).

This functionality is important for the support of the following clang
command line options in the New Offload Model:

-ftarget-compile-fast
-ftarget-export-symbols
-ftarget-register-alloc-mode

This patch fixes a handling of compile/link options encoded in images. Also the patch fixes a redirection to backend compiler and offload wrapper depending on the Triple (AOT or JIT). This functionality is important for the support of the following clang command line options in the New Offload Model: * -ftarget-compile-fast * -ftarget-export-symbols * -ftarget-register-alloc-mode

maksimsab · 2025-08-12T13:24:32Z

@asudarsa Friendly reminder :)

Failed tests aren't related to the patch. They will be fixed after rebasing.

maarquitos14 · 2025-09-01T12:38:53Z

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

+    StringRef CompileOptions = OB.getString("compile-opts");
+    StringRef LinkOptions = OB.getString("link-opts");
+    if (!CompileOptions.empty())
+      SYCLCompileOptionsFromImage = CompileOptions.str();


What if different files have different compile options? They will be overwritten, won't they? Is this possible?

maarquitos14 · 2025-09-01T12:39:00Z

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

+    if (!CompileOptions.empty())
+      SYCLCompileOptionsFromImage = CompileOptions.str();
+    if (!LinkOptions.empty())
+      SYCLLinkOptionsFromImage = LinkOptions.str();


Same than compile options.

maarquitos14 · 2025-09-01T12:40:15Z

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

+  StringRef OptC = SYCLCompileOptionsFromImage;
+  OptC.split(CmdArgs, " ", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+  OptC = Args.getLastArgValue(
+      OPT_sycl_backend_compile_options_EQ); // TODO: test this.


Suggested change

OPT_sycl_backend_compile_options_EQ); // TODO: test this.

OPT_sycl_backend_compile_options_EQ); // TODO: test this.

Is the TODO still necessary?

maarquitos14 · 2025-09-01T12:41:22Z

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

+  if (Target.empty())
+    return createStringError(
+        inconvertibleErrorCode(),
+        "can't wrap SYCL image. -triple argument is missed.");


Suggested change

"can't wrap SYCL image. -triple argument is missed.");

"can't wrap SYCL image. -triple argument is missing.");

maarquitos14 · 2025-09-01T12:43:27Z

sycl/test-e2e/NewOffloadDriver/compile_options.cpp

@@ -0,0 +1,49 @@
+// Test checks that compile options are transfared to backend in the New


Suggested change

// Test checks that compile options are transfared to backend in the New

// Test checks that compile options are transferred to backend in the New

maarquitos14 · 2025-09-01T12:46:20Z

sycl/test-e2e/NewOffloadDriver/compile_options.cpp

+// RUN: env SYCL_UR_TRACE=2 %{run} %t_with.out 2>&1 | FileCheck --check-prefix=CHECK-INTEL-GPU-WITH %s
+
+// CHECK-INTEL-GPU-WITH: <--- urProgramBuild
+// CHECK-INTEL-GPU-WITH-SAME: -igc_opts 'PartitionUnit=1,SubroutineThreshold=50000'


Not sure I understand this check. Can you explain why we expect this?

srividya-sundaram · 2025-09-02T22:54:41Z

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

@@ -158,6 +158,10 @@ static SYCLBIN::BundleState SYCLBINState = SYCLBIN::BundleState::Input;

 static SmallString<128> OffloadImageDumpDir;

+static std::string SYCLCompileOptionsFromImage;


Any specific reason why this is a std::string and not a StringRef?

Data-owning containers are more reliable against use-after-free bugs.

srividya-sundaram · 2025-09-02T23:08:22Z

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

+  llvm::Triple T(Target);
+  bool isJIT = !(T.getSubArch() == llvm::Triple::SPIRSubArch_gen ||
+                 T.getSubArch() == llvm::Triple::SPIRSubArch_x86_64);
+  SmallString<0> CompileOptions;


Any reason this is not a StringRef instead, given that you are assigning a StringRef value in L1077?

The mentioned StringRef refers to a temporary string and the data should be stored in a data-owning container.
In fact, my code contains the bug use-after-free and it should be fixed.

maksimsab had a problem deploying to WindowsCILock July 24, 2025 14:32 — with GitHub Actions Failure

maksimsab force-pushed the compile_opts branch 3 times, most recently from 6c4992c to 1b4dcc8 Compare July 29, 2025 16:18

maksimsab temporarily deployed to WindowsCILock July 29, 2025 16:19 — with GitHub Actions Inactive

maksimsab temporarily deployed to WindowsCILock July 29, 2025 17:17 — with GitHub Actions Inactive

maksimsab force-pushed the compile_opts branch from 1b4dcc8 to 6b3ad76 Compare July 30, 2025 12:09

maksimsab temporarily deployed to WindowsCILock July 30, 2025 12:09 — with GitHub Actions Inactive

maksimsab temporarily deployed to WindowsCILock July 30, 2025 12:39 — with GitHub Actions Inactive

maksimsab force-pushed the compile_opts branch from 6b3ad76 to 57e32c4 Compare July 30, 2025 12:58

maksimsab temporarily deployed to WindowsCILock July 30, 2025 12:58 — with GitHub Actions Inactive

maksimsab temporarily deployed to WindowsCILock July 30, 2025 13:33 — with GitHub Actions Inactive

maksimsab force-pushed the compile_opts branch from 57e32c4 to b20f88d Compare July 30, 2025 14:23

maksimsab had a problem deploying to WindowsCILock July 30, 2025 14:23 — with GitHub Actions Failure

maksimsab temporarily deployed to WindowsCILock July 30, 2025 14:54 — with GitHub Actions Inactive

maksimsab marked this pull request as ready for review July 31, 2025 11:54

maksimsab requested review from a team as code owners July 31, 2025 11:54

maksimsab requested a review from asudarsa July 31, 2025 11:54

maarquitos14 reviewed Sep 1, 2025

View reviewed changes

srividya-sundaram reviewed Sep 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NewOffloadModel] Fix transition of compile options from image to SYCL Offload Wrapper #19579

[NewOffloadModel] Fix transition of compile options from image to SYCL Offload Wrapper #19579

Uh oh!

maksimsab commented Jul 24, 2025 •

edited

Loading

Uh oh!

maksimsab commented Aug 12, 2025

Uh oh!

maarquitos14 Sep 1, 2025

Uh oh!

maarquitos14 Sep 1, 2025

Uh oh!

maarquitos14 Sep 1, 2025

Uh oh!

maarquitos14 Sep 1, 2025

Uh oh!

maarquitos14 Sep 1, 2025

Uh oh!

maarquitos14 Sep 1, 2025

Uh oh!

srividya-sundaram Sep 2, 2025

Uh oh!

maksimsab Sep 8, 2025

Uh oh!

srividya-sundaram Sep 2, 2025

Uh oh!

maksimsab Sep 8, 2025

Uh oh!

Uh oh!

	OPT_sycl_backend_compile_options_EQ); // TODO: test this.
	OPT_sycl_backend_compile_options_EQ); // TODO: test this.

	"can't wrap SYCL image. -triple argument is missed.");
	"can't wrap SYCL image. -triple argument is missing.");

		@@ -0,0 +1,49 @@
		// Test checks that compile options are transfared to backend in the New

		@@ -158,6 +158,10 @@ static SYCLBIN::BundleState SYCLBINState = SYCLBIN::BundleState::Input;

		static SmallString<128> OffloadImageDumpDir;

		static std::string SYCLCompileOptionsFromImage;

[NewOffloadModel] Fix transition of compile options from image to SYCL Offload Wrapper #19579

Are you sure you want to change the base?

[NewOffloadModel] Fix transition of compile options from image to SYCL Offload Wrapper #19579

Uh oh!

Conversation

maksimsab commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maksimsab commented Aug 12, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maksimsab commented Jul 24, 2025 •

edited

Loading