[SYCL][clang-linker-wrapper] Fix argument passing to ocloc. #20270

maarquitos14 · 2025-10-02T14:06:23Z

ocloc -options args need to be comma separated, e.g. -options -g,-cl-opt-disable. Otherwise, only the first arg is processed by ocloc as an arg for -options, and the rest are processed as standalone flags, possibly leading to errors. Another possibility would be -options "-g -cl-opt-disable", but quotes might cause escaping issues.

sarnex · 2025-10-02T14:09:14Z

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

+    // "-g,-cl-opt-disable"`. Otherwise, only the first arg is processed by
+    // ocloc as an arg for -options, and the rest are processed as standalone
+    // flags, possibly leading to errors.
+    std::pair<StringRef, StringRef> OptionsArgs = OptC.split("-options ");


i've never seen this before, i';ve always used quotes and that works -options "-g -cl-take-address -whatever", does it really not work?

That's another option, but it needs all the args to be within the quotes. Without this patch, the args for the example would be passed as -options "-g" -cl-opt-disable. That breaks ocloc, because it processes cl-opt-disable as a standalone flag, and errors out. I prefer to go with commas than playing with quotes, because they can cause weird escape issues, but I'm open to try that if you prefer it.

no i agree commas is better if it works, i didnt even know it worked, cool

Do you want me to update the description to include the alternative you mentioned and why I/we prefer to go with commas?

sure, just a one liner might be useful like 'Quotes would also work, but then we get into escaping issues"

sarnex · 2025-10-02T14:23:07Z

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

  StringRef OptC =
      Args.getLastArgValue(OPT_sycl_backend_compile_options_from_image_EQ);
-  OptC.split(CmdArgs, " ", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+  if (!IsCPU) {


nit: can we swap the branches so its not if(!something) else something and its if(something) else something?

Of course. Will do all the changes in one commit.

sarnex · 2025-10-02T14:24:08Z

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

+    // Only add if not empty, an empty arg can lead to ocloc errors.
+    if (!OptionsArgs.first.empty())
+      CmdArgs.push_back(OptionsArgs.first);
+    if (!OptionsArgs.second.empty()) {


is it possible OptionsArgs.first is empty and OptionsArgs.second is not empty?

Yes, actually, my test case behaved like that. The way this split function works is that OptionsArgs.first will be everything before the separator (-options in our case), and OptionsArgs.second will be everything after the separator. My test case was -options -g -cl-opt-disable, so the first is empty because there is nothing before the separator.

ok i see, but do we need to handle the case where there are some non-options related arguments after -options, or are we saying anything after -options must be part of -options?

I did some investigation on that, I didn't find any case where we add anything else after -options that doesn't belong to options. The only place I found where we use pass -options is

llvm/clang/lib/Driver/ToolChains/SYCL.cpp

Line 1975 in dfc06dd

CmdArgs.push_back("-options");

. If you can come up with an example that wouldn't work in this case, I'm happy to take a look at it. I tried myself, but didn't find a way to make the driver pass args that don't belong to -options to try this scenario.

no i think that assumption is correct actually

sarnex · 2025-10-02T14:27:10Z

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

+    // "-g,-cl-opt-disable"`. Otherwise, only the first arg is processed by
+    // ocloc as an arg for -options, and the rest are processed as standalone
+    // flags, possibly leading to errors.
+    std::pair<StringRef, StringRef> OptionsArgs = OptC.split("-options ");


nit: we could use structured bindings here to get some better variable names, eg

auto [OptionStr, OptionValues] = OptC.split("-options ");

I thought about it, just not sure if some of our older CI systems (RHEL 7.6 I think) supports that. Do we use it anywhere else? Is it okay? I'm happy to go change it if it works.

LLVM requires c++17 to build and this feature is in c++17 so we should be fine

sarnex · 2025-10-02T14:28:43Z

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

+    // flags, possibly leading to errors.
+    std::pair<StringRef, StringRef> OptionsArgs = OptC.split("-options ");
+    // Only add if not empty, an empty arg can lead to ocloc errors.
+    if (!OptionsArgs.first.empty())


i thought OptionArgs.first was -options but it seems we push that here and then `options" explicitly in the next branch, so probably im mistaken

I explained in #20270 (comment). I thought so too when I first used the function, but you can see here the llvm doc: https://llvm.org/doxygen/classllvm_1_1StringRef.html#a0320b2a5a6d440bf4479a02e78cf5ca7

Given it's caused confusion to both you and me, I'll better add a comment in the code explaining, so nobody needs to go find the doc.

cool, for me the confusion was around the uncommon separate (usually , or something), if we explain that we're splitting around that i think it's clearer

sarnex · 2025-10-02T18:16:29Z

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

  StringRef OptC =
      Args.getLastArgValue(OPT_sycl_backend_compile_options_from_image_EQ);
-  if (!IsCPU) {
+  if (IsCPU) {


any way to add a test?

There should be already. The case I'm fixing here is because a test failed downstream. Let me double-check if there are also for CPU case.

So for the CPU case, I think this test covers it: https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/NewOffloadDriver/cpu.cpp
For the GPU case, https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/SYCLBIN/simple_kernel_aot. was the one failing downstream, so it covers the GPU case.

[SYCL][clang-linker-wrapper] Fix argument passing to ocloc.

9496bb7

maarquitos14 requested a review from a team as a code owner October 2, 2025 14:06

maarquitos14 temporarily deployed to WindowsCILock October 2, 2025 14:06 — with GitHub Actions Inactive

sarnex reviewed Oct 2, 2025

View reviewed changes

maarquitos14 had a problem deploying to WindowsCILock October 2, 2025 15:05 — with GitHub Actions Failure

maarquitos14 temporarily deployed to WindowsCILock October 2, 2025 15:05 — with GitHub Actions Inactive

Address code review.

9949bb8

maarquitos14 temporarily deployed to WindowsCILock October 2, 2025 18:04 — with GitHub Actions Inactive

sarnex reviewed Oct 2, 2025

View reviewed changes

maarquitos14 temporarily deployed to WindowsCILock October 2, 2025 18:41 — with GitHub Actions Inactive

maarquitos14 requested a review from sarnex October 3, 2025 13:42

sarnex approved these changes Oct 3, 2025

View reviewed changes

steffenlarsen merged commit c10603b into intel:sycl Oct 3, 2025
36 of 37 checks passed

[SYCL][clang-linker-wrapper] Fix argument passing to ocloc. #20270

[SYCL][clang-linker-wrapper] Fix argument passing to ocloc. #20270

Uh oh!

Conversation

maarquitos14 commented Oct 2, 2025 • 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!

maarquitos14 Oct 2, 2025 • 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maarquitos14 commented Oct 2, 2025 •

edited

Loading

maarquitos14 Oct 2, 2025 •

edited

Loading