[SYCL] Add option forwarding from clang-linker-wrapper tool to clang-sycl-linker by srividya-sundaram · Pull Request #21958 · intel/llvm

srividya-sundaram · 2026-05-07T23:04:11Z

This is the first patch in a series that moves SYCL device code linking logic from clang-linker-wrapper into the clang-sycl-linker tool.
I’m continuing the work from where Yixing left off.

At the moment, the tests only verify that the options are forwarded to clang-linker-wrapper.
Tests validating clang-sycl-linker invocations will be added in subsequent PRs.

Added infrastructure from PR 21692 + tests for option forwarding.

maksimsab · 2026-05-08T14:02:42Z

More general comment.
I see that your PR targets 2 independent goals:

Add option forwarding
Remove deprecated device library options

In cases like that the suggested approach is:

The patch should:
* be an isolated change. Independent changes should be submitted as separate patches as this makes reviewing easier.

https://llvm.org/docs/Contributing.html#how-to-submit-a-patch

Please, use this approach in your patches.

YuriPlyakhin

could you please update title?

YuriPlyakhin · 2026-05-13T22:14:22Z

@@ -0,0 +1,42 @@
+// Test that clang-linker-wrapper correctly invokes clang-sycl-linker when --use-clang-sycl-linker is enabled


this test looks similiar to the one I see in community: https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/linker-wrapper.c

Other similar tests that are programming-model or target specific:

https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/linker-wrapper-hip-amdgcnspirv.c

https://github.com/llvm/llvm-project/blob/e721580311e3988e767ec3aaa06bce025f72ff7e/clang/test/Driver/hipspv-toolchain.hip#L55

Do you think it would be possible to add test like you wrote here to community first aligned with naming and style? (like linker-wrapper-sycl.cpp), and then add modifications in downstream on top of what is in upstream?

I want to minimize our back-and-forth between downstream and community and submit to community first if that makes sense.

YuriPlyakhin · 2026-05-13T22:21:20Z

 // REQUIRES: spirv-registered-target
 //
-// Test the dry run of a simple case to link two input files.
+// Create bitcode files for clang-sycl-linker direct tests


I'm moving this test to Tooling in community, pls, see here: llvm/llvm-project#197566
I think it might make sense to wait while this patch lands, before modifying it in downstream.

Also, I'd prefer to minimize updating comments like this. Original comment explains the test case, not sure why it was removed.

Upd: after I reviewed modifications to entire file, I think this current file should not be modified at all, and instead all additional tests should be added as a new driver test file. Current test is testing clang-sycl-linker, you want to add clang driver test.

YuriPlyakhin · 2026-05-13T22:23:39Z

+// Create bitcode files for clang-sycl-linker direct tests
 // RUN: %clangxx -emit-llvm -c -target spirv64 %s -o %t_1.bc
 // RUN: %clangxx -emit-llvm -c -target spirv64 %s -o %t_2.bc
+// Create SYCL offload object files for driver tests


I'd prefer to keep this test as clang-sycl-linker test and driver tests should be added separately.

YuriPlyakhin · 2026-05-13T22:29:03Z

 // RUN: | FileCheck %s --check-prefix=NOTARGET
 // NOTARGET: Target triple must be specified
+//
+// Test that driver options are correctly passed through clang-linker-wrapper.


I think this should be a new test, similar to

https://github.com/llvm/llvm-project/blob/e91b574832478626b621925e0d018ed2514a9959/clang/test/Driver/sycl-offload-jit-unix.cpp

https://github.com/llvm/llvm-project/blob/e91b574832478626b621925e0d018ed2514a9959/clang/test/Driver/sycl-offload-jit-xarch.cpp#L8

https://github.com/llvm/llvm-project/blob/e91b574832478626b621925e0d018ed2514a9959/clang/test/Driver/hipspv-toolchain.hip#L40

And if anything from this test can be submitted upstream, that should be done first (but looking at what is being checked here, it seems this test indeed belongs to downstream for now).

YuriPlyakhin · 2026-05-13T22:38:31Z

+// CHECK-DEFAULT: clang-linker-wrapper
+// CHECK-DEFAULT-NOT: "--use-clang-sycl-linker"
+//
+// Test that --use-clang-sycl-linker flag is passed through


It seems some tests in this file duplicate tests in previous file. For example what this test is testing differently comparing to the one in clang/test/Driver/clang-sycl-linker-test.cpp line 123

I think there are some other duplications as well

YuriPlyakhin · 2026-05-13T22:56:37Z

+
 Expected<StringRef> clang(ArrayRef<StringRef> InputFiles, const ArgList &Args,
-                          bool IsSYCLKind = false) {
+                          uint16_t ActiveOffloadKindMask = 0) {


is it possible to avoid divergence like this with the community code? In upstream, ActiveOffloadKindMask is not a default parameter. Is there a strong reason to make it a default parameter in this code base?
I checked in all calls to clang in this file, we anyway pass this param explicitly.

YuriPlyakhin · 2026-05-13T22:57:40Z

+// Set via --use-clang-sycl-linker flag.
+// TODO: Remove this flag once the linking and backend compilation have been
+// fully migrated to clang-sycl-linker and make it the default behavior.
+bool UseClangSYCLLinker = false;


Suggested change

bool UseClangSYCLLinker = false;

static bool UseClangSYCLLinker = false;

YuriPlyakhin · 2026-05-13T23:01:12Z

+      OPT_bitcode_library_EQ,
+  };
+  for (OptSpecifier Opt : ValueOpts) {
+    if (const opt::Arg *A = Args.getLastArg(Opt)) {


OPT_bitcode_library_EQ can be provided multiple times, right? getLastArg will use only last value, so previous libraries would be dropped.

YuriPlyakhin · 2026-05-13T23:05:26Z

+      OPT_sycl_remove_unused_external_funcs,
+      OPT_no_sycl_remove_unused_external_funcs,


since we are using getLastArg on this array, initializing DirectOpts with both yes/no variants of the option may cause forwarding the wrong option (like yes-variant is provided last in the command line, but we are checking for no-variant in the loop last).
Should we use something like this instead?
Args.hasFlag(OPT_pos, OPT_neg, default)
or
getLastArg(OPT_pos, OPT_neg)

YuriPlyakhin · 2026-05-13T23:11:54Z

    CmdArgs.push_back("-c");
  }

+  if (UseClangSYCLLinker && (ActiveOffloadKindMask & OFK_SYCL)) {


this ordering of the code is causing nativecpu to use clang-sycl-linker. Does this pass work? Are there any tests for this?
same question for nvptx pass.

YuriPlyakhin · 2026-05-13T23:15:14Z

+/// invocation).
+/// \param Triple The target triple for the device.
+/// \param Arch The target architecture (e.g., "pvc", "graniterapids").
+static void appendClangSYCLLinkerArgs(const ArgList &Args,


This is sycl-specific function, should it be placed to sycl namespace?

YuriPlyakhin · 2026-05-13T23:17:39Z

+    }
+  }
+
+  static const OptSpecifier ValueOpts[] = {


are all options which we are forwarding to clang-sycl-linker supported by it?

YuriPlyakhin

this pr needs some more work.

srividya-sundaram marked this pull request as ready for review May 8, 2026 00:27

srividya-sundaram requested review from a team as code owners May 8, 2026 00:27

YuriPlyakhin reviewed May 8, 2026

View reviewed changes

Comment thread clang/tools/clang-sycl-linker/ClangSYCLLinker.cpp

Only have option forwarding and test changes.

20f67e1

srividya-sundaram force-pushed the clw-to-csl branch from 8d6725d to 20f67e1 Compare May 8, 2026 17:01

srividya-sundaram requested a review from YuriPlyakhin May 8, 2026 17:03

YuriPlyakhin reviewed May 8, 2026

View reviewed changes

srividya-sundaram changed the title ~~[SYCL] Add option forwarding and remove deprecated device library options~~ [SYCL] Add option forwarding from clang linker wrapper to clang sycl linker May 8, 2026

srividya-sundaram changed the title ~~[SYCL] Add option forwarding from clang linker wrapper to clang sycl linker~~ [SYCL] Add option forwarding from clang-linker-wrapper tool to clang-sycl-linker May 8, 2026

srividya-sundaram requested a review from YuriPlyakhin May 8, 2026 18:28

YuriPlyakhin reviewed May 8, 2026

View reviewed changes

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

maksimsab requested changes May 12, 2026

View reviewed changes

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

srividya-sundaram added 2 commits May 12, 2026 17:57

Check option forwarding to CSL.

0b40efc

Fix clang format.

f5327ea

mdtoguchi reviewed May 13, 2026

View reviewed changes

Comment thread clang/test/Driver/clang-sycl-linker-test.cpp Outdated

mdtoguchi reviewed May 13, 2026

View reviewed changes

Comment thread clang/test/Driver/clang-sycl-linker-test.cpp Outdated

mdtoguchi reviewed May 13, 2026

View reviewed changes

Comment thread clang/test/Driver/clang-sycl-linker-test.cpp Outdated

mdtoguchi reviewed May 13, 2026

View reviewed changes

Comment thread clang/test/Driver/clang-sycl-linker-test.cpp Outdated

Migrate to uint16_t ActiveOffloadKindMask and update test.

50fddb1

srividya-sundaram requested review from YuriPlyakhin, bader, maksimsab and mdtoguchi May 13, 2026 18:32

YuriPlyakhin reviewed May 13, 2026

View reviewed changes

srividya-sundaram mentioned this pull request May 13, 2026

[SYCL] [ClangSYCLLinker] Add full sycl-post-link functionality to clang-sycl-linker #21977

Draft

YuriPlyakhin reviewed May 13, 2026

View reviewed changes

YuriPlyakhin requested changes May 13, 2026

View reviewed changes

		@@ -0,0 +1,42 @@
		// Test that clang-linker-wrapper correctly invokes clang-sycl-linker when --use-clang-sycl-linker is enabled

	bool UseClangSYCLLinker = false;
	static bool UseClangSYCLLinker = false;

		OPT_sycl_remove_unused_external_funcs,
		OPT_no_sycl_remove_unused_external_funcs,

Conversation

srividya-sundaram commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

maksimsab commented May 8, 2026

Uh oh!

YuriPlyakhin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

YuriPlyakhin May 13, 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!

YuriPlyakhin May 13, 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!

YuriPlyakhin May 13, 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!

YuriPlyakhin May 13, 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!

YuriPlyakhin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

srividya-sundaram commented May 7, 2026 •

edited

Loading

YuriPlyakhin May 13, 2026 •

edited

Loading

YuriPlyakhin May 13, 2026 •

edited

Loading

YuriPlyakhin May 13, 2026 •

edited

Loading

YuriPlyakhin May 13, 2026 •

edited

Loading