Repository navigation
[SYCL][clang-linker-wrapper] Wrap all architectures in one module for -fno-sycl-rdc - #23492
Conversation
… -fno-sycl-rdc The -fno-sycl-rdc compile step (--emit-fatbin-only) wrapped the images of each target group separately and then expected a single output, so a compile for several architectures of one triple, e.g. -fsycl-targets=intel_gpu_pvc,intel_gpu_dg2, failed with "Expect single output from the SYCL device linker". Collect the images of all groups and wrap them into a single wrapper module, as is done for the other offload kinds. The per-architecture information, such as the compile_target, is carried by each image. Inputs with different target triples, which the driver already rejects with -fno-sycl-rdc, are diagnosed rather than labelled with one triple.
srividya-sundaram
added
the
new-offload-model
iclsrc
left a comment
iclsrc
left a comment
There was a problem hiding this comment.
This PR changes the -fno-sycl-rdc compile step so the modules from all architecture groups are wrapped into one wrapper module, not one module per group. The wrapping now happens after the parallel loop. Modules are collected per input group, so their order is stable, and mixed target triples are rejected. Overall the logic looks sound. I found no critical issues, only two minor points below.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open findings
LinkerArgsdoes not contain-triple,Triplewill be empty andNoRDCTripleremains empty.… · NewNoRDCModulesvia pointer arithmetic on a range-loop reference is hard to read and relies… · Newoffload-wrapper:occurrences after the first match, which… · NewImageMtx(presumably also guarding sharedImagesstate) to protect… · New
What changed in this PR
Updates clang-linker-wrapper’s SYCL --no-sycl-rdc flow to wrap multiple architecture images into a single wrapper module, avoiding “Expect single output from the SYCL device linker” failures for multi-arch inputs.
Changes:
- Defer
--no-sycl-rdcwrapping until all inputs are processed, then emit a single wrapper module across architectures. - Add a diagnostic for mixed SYCL target triples under
--no-sycl-rdc. - Extend the test suite to cover multi-architecture wrapping and mixed-triple rejection.
| File | Description |
|---|---|
| clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp | Defers --no-sycl-rdc wrapping, aggregates modules across architectures, and enforces single target triple. |
| clang/test/OffloadTools/clang-linker-wrapper/linker-wrapper-sycl.cpp | Adds coverage for multi-arch single-wrapper emission and mixed-triple error behavior. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Also diagnose a missing triple, use a dedicated mutex and explain why the global arguments are used for wrapping.
|
The Windows BMG failure is unrelated to this PR. |
sarnex
left a comment
sarnex
left a comment
There was a problem hiding this comment.
sorry, i dont really understand this change so i think we will need to wait for yury
Does it match old offload model behavior? |
|
Do we have SYCL E2E test that would demonstrate that no-rdc fat binary (2 architectures, e.g. "" and "pvc" or something) works end to end (compilation + runtime)? My concern is that without runtime check, we can easily write anything into wrapper, and nobody catches that wrapper is incorrect. |
Collect the per-group modules without a lock, make the target triple a required argument of wrapSYCLBinariesFromFile, and check in the test that the images of both architectures reach the one wrapper module.
No. The old model accepts it: it runs one |
It means, this PR is not final then for no-rdc in new offload model. Please, submit issue to enable multiple triples. |
|
CI failures are unrelated to this PR. |
Added AOT/early_aot_multi_arch.cpp |
The bundler, runtime and sanitizer library fixes have landed in sycl (intel#23491, intel#23493, intel#23494), so their copies are dropped here. The linker wrapper fix is updated to the version in intel#23492.
@srividya-sundaram , are there github issues for those failures? If not, could you please file them? |
#22293 is already filed for |
YuriPlyakhin
left a comment
YuriPlyakhin
left a comment
There was a problem hiding this comment.
Other than few nits, LGTM. Thanks!
sarnex
left a comment
sarnex
left a comment
There was a problem hiding this comment.
no flags from me but my review is not worth much here :P
Without -fsycl, the Windows link uses the static C runtime, but the object was compiled for the dynamic one (__imp__wassert is unresolved).
The linker wrapper fix has landed in sycl (intel#23492), so its copy is dropped here.
Fixes #23488.
With the new offloading model,
-fno-sycl-rdc -cfor several architectures of one triple (e.g.-fsycl-targets=intel_gpu_pvc,intel_gpu_dg2) failed with "Expect single output from the SYCL device linker", because each architecture's device image was put into its own offload wrapper module.clang-linker-wrappernow puts all architectures' images into one wrapper module, which the host compilation embeds. All images in a wrapper module are labeled with one target triple, so SYCL inputs with different triples are now diagnosed before any device compilation. The driver already rejects that case; supporting it is tracked in #23526.New E2E test
AOT/early_aot_multi_arch.cppbuilds a-fno-sycl-rdc -cobject for two GPUs and runs it, checking that the runtime picks the right image.Split out of #23441, whose tests need it.