Skip to content

[SYCL] Forward AOT backend/linker options as individual tokens#22645

Merged
uditagarwal97 merged 9 commits into
intel:syclfrom
bader:sycl-aot-token-fix
Jul 18, 2026
Merged

[SYCL] Forward AOT backend/linker options as individual tokens#22645
uditagarwal97 merged 9 commits into
intel:syclfrom
bader:sycl-aot-token-fix

Conversation

@bader

@bader bader commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Route -Xsycl-target-backend and -Xsycl-target-linker options through
--device-compiler=/--device-linker= uniformly for JIT and AOT SYCL
targets, one flag per token instead of joining tokens into a single
string. clang-linker-wrapper now keeps CLI-supplied AOT options as a
token list end-to-end and appends them to ocloc/opencl-aot argv
individually, avoiding a lossy re-split of values with embedded
spaces. JIT targets keep the existing string-joining behavior for the
image's compile-opts/link-opts properties.

bader added 2 commits July 15, 2026 10:14
Route -Xsycl-target-backend and -Xsycl-target-linker options through
--device-compiler=/--device-linker= uniformly for JIT and AOT SYCL
targets, one flag per token instead of joining tokens into a single
string. clang-linker-wrapper now keeps CLI-supplied AOT options as a
token list end-to-end and appends them to ocloc/opencl-aot argv
individually, avoiding a lossy re-split of values with embedded
spaces. JIT targets keep the existing string-joining behavior for the
image's compile-opts/link-opts properties.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts SYCL option forwarding so that -Xsycl-target-backend / -Xsycl-target-linker are forwarded to clang-linker-wrapper as one --device-compiler= / --device-linker= flag per token (instead of joining tokens into a single space-separated string), and updates clang-linker-wrapper to preserve and forward those tokens end-to-end for AOT backends.

Changes:

  • Emit one --device-compiler=...=<token> / --device-linker=...=<token> per translated SYCL backend/link option in the driver.
  • Keep CLI-supplied AOT options as a token list in clang-linker-wrapper and append them directly to ocloc / opencl-aot argv (avoiding lossy re-splitting).
  • Update driver/wrapper tests to reflect the new per-token forwarding behavior (including AOT coverage).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp Thread AOT device args as token lists through the SYCL pipeline and append to AOT tool invocations.
clang/lib/Driver/ToolChains/Clang.cpp Forward SYCL backend/linker options to clang-linker-wrapper as one flag per token.
clang/test/Driver/sycl-offload.cpp Update checks to expect per-token --device-compiler/--device-linker forwarding.
clang/test/Driver/sycl-offload-new-driver.cpp Add coverage ensuring AOT targets also forward backend/linker options as separate tokens.
clang/test/Driver/clang-linker-wrapper.cpp Extend wrapper tests for split -device reconstruction and multi-arg AOT option forwarding.

Comment on lines +1939 to +1947
std::string AOTOptions;
if (Triple.isSPIRAOT()) {
AOTOptions = CompileLinkOptions.first;
if (!CompileLinkOptions.second.empty()) {
if (!AOTOptions.empty())
AOTOptions += ' ';
AOTOptions += CompileLinkOptions.second;
}
}
bader added 7 commits July 15, 2026 15:46
Update the CHK-NO-CMDS-AOT-GEN-LINKERARG test to assert on the actual
ocloc invocation instead of an unrelated sycl-post-link line, matching
the new one-CLI-token-equals-one-argv-entry contract for
--device-compiler=/--device-linker=. Remove CHK-NO-CMDS-AOT-GEN (relied
on a single-token "-device pvc" CLI value that is no longer split, and
is unreachable via the real driver since -Xsycl-target-backend already
tokenizes it into separate flags) and CHK-MULTI-DEVICE-LINKER-AOT
(duplicated the same reconstruction behavior as the fixed test).
-Xsycl-target-backend/-Xsycl-target-linker require the driver and
clang-linker-wrapper to parse and re-serialize ocloc's own option
syntax (e.g. the literal "-options " split in addOclocOptions), which
is fragile — appended link-opts can be silently absorbed into the
-options value. Note the deprecation/removal of these options as a
follow-up to avoid this class of parsing entirely.
Add a comment cross-referencing Driver::getOffloadArchs, which performs
a similar "-device" token detection on the driver side for spir64_gen
Arch selection, so a future edit to either detector's handling of
-Xsycl-target-backend syntax prompts a check of the other.
…Module

Triple parsing, AOTOptions/BackendOptions derivation, and the
-device detection in IsDevicePassedWithSyclTargetBackend were
recomputed on every call to postLinkProcessModule, even though the
value is identical across all split modules sharing a triple. Moved
the computation up to runSYCLOffloadingPipeline, which now computes
it once before looping over Modules and passes the results down.
invokeBackendForSYCLDevice and compileDeviceAndBundle are single-caller
helpers whose only call sites already pass AOTDeviceArgs explicitly, so
the `= {}` defaults were unreachable and misleadingly implied some
caller omits the argument.
BuildArgs is joined into a single space-separated compile-opts=/link-opts=
string before being embedded in the offload image, then later re-split on
spaces by clang-linker-wrapper for AOT triples. This corrupts any token
containing an embedded space unless it happens to fall inside the
pre-existing "-options \"...\"" wrapper convention. The CLI-supplied
counterpart (--device-compiler=/--device-linker=) already avoids this by
forwarding individual tokens via AOTDeviceArgs.
linkAndWrapDeviceFiles() and runSYCLOffloadingPipeline() each decide
independently whether CLI-supplied AOT options go through the token
vector or the flat-string path, with nothing enforcing the two
isSPIRAOT() checks stay in agreement.
@bader
bader marked this pull request as ready for review July 16, 2026 00:26
@bader
bader requested review from a team as code owners July 16, 2026 00:26
@bader

bader commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

ping

CmdArgs.push_back("-options");
CmdArgs.push_back(AfterOptions);
}
llvm::append_range(CmdArgs, AOTDeviceArgs);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any concern on ordering of the options? AOTDeviceArgs will always fall after the other options.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of any issues with ocloc argument ordering.
Also, I don't think my patch changes the ordering.

Expected<std::pair<std::string, std::string>> CompileLinkOptionsOrErr =
extractSYCLCompileLinkOptions(Input);
if (!CompileLinkOptionsOrErr)
return CompileLinkOptionsOrErr.takeError();
std::pair<std::string, std::string> &CompileLinkOptions =
*CompileLinkOptionsOrErr;
// Append device compiler and linker options passed via
// -device-compiler= and -device-linker= to clang-linker-warpper,
// together with options extracted from the image.
StringRef DeviceCompilerArgs =
LinkerArgs.getLastArgValue(OPT_compiler_arg_EQ);
if (!DeviceCompilerArgs.empty()) {
CompileLinkOptions.first += " ";
CompileLinkOptions.first += DeviceCompilerArgs;
}
StringRef DeviceLinkerArgs =
LinkerArgs.getLastArgValue(OPT_linker_arg_EQ);
if (!DeviceLinkerArgs.empty()) {
CompileLinkOptions.second += " ";
CompileLinkOptions.second += DeviceLinkerArgs;
}

Do you think there is a problem with the order of ocloc arguments?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose not - dividing up the backend args and device args shouldn't provide opportunity for potential override behaviors

@mdtoguchi mdtoguchi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK by me - lots of TODO and FIXME items that need to be tracked.

@sys-ce-bb

Copy link
Copy Markdown
Contributor

@intel/llvm-gatekeepers please consider merging

@uditagarwal97
uditagarwal97 merged commit dd9abc1 into intel:sycl Jul 18, 2026
31 checks passed
// (happen when AOT device is specified via -Xsycl-target-backend '-device pvc' in clang),
// the target is not passed to sycl-post-link for filtering.
// RUN: clang-linker-wrapper -sycl-embed-ir --bitcode-library=spir64_gen-unknown-unknown=%t1.devicelib.bc -sycl-post-link-options="SYCL_POST_LINK_OPTIONS" -llvm-spirv-options="LLVM_SPIRV_OPTIONS" "--host-triple=x86_64-unknown-linux-gnu" "--device-compiler=spir64_gen-unknown-unknown=-device pvc" "--linker-path=/usr/bin/ld" "--" HOST_LINKER_FLAGS "-dynamic-linker" HOST_DYN_LIB "-o" "a.out" HOST_LIB_PATH HOST_STAT_LIB %t1.o --dry-run 2>&1 | FileCheck -check-prefix=CHK-NO-CMDS-AOT-GEN %s
// CHK-NO-CMDS-AOT-GEN: sycl-post-link{{.*}} SYCL_POST_LINK_OPTIONS -o {{[^,]*}}.table {{.*}}.bc

@YuriPlyakhin YuriPlyakhin Jul 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removed CHK-NO-CMDS-AOT-GEN check verified that -device suppresses the intel_gpu_... prefix passed to sycl-post-link. The IsDevicePassedWithSyclTargetBackend path is now untested.
Please add a sycl-post-link{{.*}} -o {{[^,]*}}.table check to the new RUN line.

// Check that in AOT case backend options are passed to ocloc and are not passed to offload wrapper
// because SYCL Runtime can't make any use of it in AOT case.
// CHECK-COMPILE-LINK-OPTS-AOT: ocloc{{.*}} -device pvc ccc ccc -output
// Check that in AOT case backend and linker options are passed to ocloc and

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR also changes AOT behavior: image-embedded link-opts are now forwarded to ocloc, whereas before only compile-opts reached the AOT backend. This is reasonable, but it's a behavior change independent of the token-splitting fix — please call it out in the PR description.

Comment on lines +129 to +134
// Check that when "-device pvc" specification, split across two separate
// --device-linker= arguments (each --device-compiler=/--device-linker= CLI
// occurrence becomes exactly one ocloc argv entry, so a multi-token value
// like "-device pvc" must be supplied as separate occurrences, not as one
// value containing an embedded space), is reconstructed correctly in the
// ocloc invocation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
// Check that when "-device pvc" specification, split across two separate
// --device-linker= arguments (each --device-compiler=/--device-linker= CLI
// occurrence becomes exactly one ocloc argv entry, so a multi-token value
// like "-device pvc" must be supplied as separate occurrences, not as one
// value containing an embedded space), is reconstructed correctly in the
// ocloc invocation.
// Check that a "-device pvc" specification split across two separate
// --device-linker= arguments (each --device-compiler=/--device-linker= CLI
// occurrence becomes exactly one ocloc argv entry, so a multi-token value
// must be supplied as separate occurrences)
// is reconstructed correctly in the ocloc invocation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants