[sarnex]

The -fsycl-link option allows creating fully-linked device objects that can be used with an arbitrary host compiler such as gcc.

There is a problem today where putting -fsycl-link fully-linked objects inside a static archive and using that static archive with a host linker results in none of the objects getting pulled in, which eventually causes a runtime assertion in getDeviceKernelInfo in the SYCL runtime as the device images are never registered.

See #23194 for more information on the failing case.

The root cause is that unless the linker is passed --whole-archive, only object files required to complete the link are linked in from the static archive, and there is no symbol dependency between the device objects in the static library and anything else, so these objects never get pulled in by the host linker.

The solution proposed in this PR is to introduce a symbol dependency between the host object and wrapped fully-linked device object, such that the linker is forced to pull in the associated device objects when the host object is passed to the linker.

The host FE adds a global constructor function __sycl_registerlib_ctor that simply just calls a function named __sycl_registerlib_<unique_identifier>, where <unique_identifier> is the unique module prefix, an existing unique identifier that can be used to match host side and device side code together.

Later, clang-linker-wrapper checks any input fat object files for undefined references to the __sycl_registerlib_<unique_identifier> symbols, and defines all of them in the wrapper module that contains the embedded devices images. The functions themselves just return void immediately.

We need to be careful in the clang-linker-wrapper step, because one host object can result in multiple device objects, and separately, -fsycl-link only allows linking one target at a time so we may be passed device images that will not be referenced in the resultant wrapped device image. The former case is managed by defining the __sycl_registerlib_<unique_identifier> functions with weak linkage and a COMDAT equal to the function name, and the latter case is managed by keeping track of the triple and arch for the undefined symbols we find in the input device images, and only defining symbols that are from a device image whose target is compatible with the wrapped device image we are creating.

This solution is consistent with CUDA's behavior when using the -dlink option. -dlink is similar to -fsycl-link, and they handle the above case in a similar way, where the FE references a symbol that is only defined in the fully-linked output of ptxas. In their case, the symbol is named __cudaRegisterLinkedBinary<unique_identifier>. See upstream implementation here. Note this only contains the FE side, as the device side linker for CUDA is done inside the closed-source ptxas, however manually analyzing the symbol table of ptxas -dlink output reveals matching behavior as proposed in this change.

HIP has a --hip-link option, but it is not similar to CUDA's -dlink nor -fsycl-link, so we can't use it as a comparison.

This PR only implements the fix for the new offload model. I will investigate old offload model support after merging this PR.

Note this change affects every new offload model object, even when not using archives or -fsycl-link.

Fixes: #23194

Co-Authored-By: Claude Sonnet 5.5 noreply@anthropic.com

[sarnex]

This code is not new, nor is the below Options.td change.
It was previously upstream code but was removed when upstream removed the old driver in llvm/llvm-project@feea42c

We just pulled down that change a couple of days ago, I am just adding it back here because I want to use it to restrict this fix.

[sarnex]

The CI failures are not related, they're already reported in #23387 and #23373

[YuriPlyakhin]

Names are recorded under the (triple, arch) of the device images embedded in each host object, but looked up with the link job's --triple/--arch. getLinkerArgs takes those from Input.front(). These don't always match. Archive members are merged into jobs using areTargetsCompatible, which treats generic as compatible with any arch. Example: main.o built for intel_gpu_pvc and libfoo.a with a member built for spir64_gen with a generic arch. The member's device code is linked into the (spir64_gen, pvc) job, but its name is stored under (spir64_gen, ""). Nothing defines it, so the host link will fail with an undefined __sycl_registerlib_uid…. That build works before this PR.

as an idea:
Instead of recording names by (triple, arch) and looking them up again with the job's --triple/--arch, could we map each image back to its host object (extractOffloadFiles already sets the buffer identifier to the host object name) and define that object's __sycl_registerlib_* names. This fixes the generic/pvc mismatch, makes the map read-only during the parallel link, and removes the need for the cross-job dedupe.

pseudocode:

// host object/member name -> its undefined __sycl_registerlib_* names.
static StringMap<SmallVector<std::string, 1>> SYCLRegisterLibNames;
...
// linkAndWrapDeviceFiles, per-job lambda: names for exactly this job's images.
SetVector<StringRef> RegisterLibNames;
for (const OffloadFile &File : Input) {
  StringRef Id = File.getBinary()->getMemoryBufferRef().getBufferIdentifier();
  auto It = SYCLRegisterLibNames.find(Id);   // read-only lookup
  if (It != SYCLRegisterLibNames.end())
    RegisterLibNames.insert(It->second.begin(), It->second.end());
}

// ...pass RegisterLibNames down to wrapSYCLBinariesFromFile and define each as weak.

The problem with this approach though is archive members are identified by member name only, so libA.a(foo.o) and libB.a(foo.o) would collide.

Something to think about...

[sarnex]

Nice find, I guess this case isn't exercised in any test in the entire SYCL suite, but that's not too surprising :P

I don't think that solution will work because we still need the triple and arch to prevent over-defining in cases where the linker is passed main.o and libfoo.a, and main.o was built for (spir64_gen, pvc) and libfoo.a contains two objects, (spir64_gen, pvc), (spir64_gen, dg2). In this case, the (spir64_gen, dg2) object goes completely unused today, and we need to main that.

Let me try to reproduce the Arch == generic case and see if I can come up with something.

[sarnex]

the Arch == generic case is fixed in 629ecd3 and added a test to lock it down in ca731f7

[YuriPlyakhin]

I don't think that solution will work because we still need the triple and arch to prevent over-defining in cases where the linker is passed main.o and libfoo.a, and main.o was built for (spir64_gen, pvc) and libfoo.a contains two objects, (spir64_gen, pvc), (spir64_gen, dg2). In this case, the (spir64_gen, dg2) object goes completely unused today, and we need to main that.

I think the per-Input approach should still handle the (pvc, dg2) archive case: if I'm not mistaken, the dg2 member would not be added to Input, if it is not compatible. It would also cover the reverse generic case (generic main + pvc member), which still fails with the current special case.

[YuriPlyakhin]

The -fsycl-link options allows

option allows

We need to be careful in the clang-linker-wrapper step, because one host object can result in multiple device objects, and separately, -fsycl-link only allows linking one target at a time so we may be passed device images that will not be referenced in the resultant wrapped device image. The former case is managed by keeping a set of the names of the __sycl_registerlib_<unique_identifier> functions we define to ensure we only process each host symbol once, and the latter case is managed by keeping track of the triple and arch for the undefined symbols we find in the input device images, and only defining symbols that corresponding with the target and arch of the wrapped device image we are creating.

Dedupe is now per module, with weak/comdat linkage, right?
symbols that correspond to

Note this change effects every new offload model object, even when not using archives or -fsycl-link.

affects

[tahonermann]

@sarnex,

I have decided to limit the fix to only the new offload model, as this limitation is fine for the customer who reported this issue, and supporting this in the old model would be more complex.

The issue this PR addresses (#23194) has also been encountered by the CMake developers working on SYCL support. They have implemented the -Wl,--whole-archive workaround, but a fix for the old offload driver would enable them to drop the workaround for compiler versions known to have a suitable fix. I don't think use of the new offload driver is a desirable solution for CMake at this time.

[sarnex]

The issue this PR addresses (#23194) has also been encountered by the CMake developers working on SYCL support. They have implemented the -Wl,--whole-archive workaround, but a fix for the old offload driver would enable them to drop the workaround for compiler versions known to have a suitable fix. I don't think use of the new offload driver is a desirable solution for CMake at this time.

Thanks, I can look into old offload model support but I would prefer to do it in a separate PR, this PR is already relatively complex with a long review history and I wouldn't want to add a new 500+ lines of product code for reviewers to review at this point.

[sarnex]

The -fsycl-link options allows

option allows

We need to be careful in the clang-linker-wrapper step, because one host object can result in multiple device objects, and separately, -fsycl-link only allows linking one target at a time so we may be passed device images that will not be referenced in the resultant wrapped device image. The former case is managed by keeping a set of the names of the __sycl_registerlib_<unique_identifier> functions we define to ensure we only process each host symbol once, and the latter case is managed by keeping track of the triple and arch for the undefined symbols we find in the input device images, and only defining symbols that corresponding with the target and arch of the wrapped device image we are creating.

Dedupe is now per module, with weak/comdat linkage, right? symbols that correspond to

Note this change effects every new offload model object, even when not using archives or -fsycl-link.

affects

Should be fixed, thanks!

[elizabethandrews]

@tahonermann can you please review this PR?

[sarnex]

@YuriPlyakhin Ping on this one if possible, thanks!

[YuriPlyakhin]

LGTM, thanks!

[sarnex]

thanks for the reviews!

[sarnex]

Ping @intel/llvm-reviewers-runtime, the only relevant changes are 2 lines of code in the LIT infra to support a new lld LIT feature, thanks!

[sarnex]

@tahonermann looks like you are the last missing reviewer, do you mind taking a look when you have a sec? thanks!

[sarnex]

I need to rebase and probably remove some tests because #23441 was just merged which removes some of the cases we need to think about here.

[sarnex]

FYI @YuriPlyakhin I removed the NewOffloadDriver/static-lib-gen-generic-arch.cpp test in f31d8a9 because that case is now a compile time error after #23441, i didn't find any other tests/comments that needed updating now that #23441 is merged

[tahonermann]

Thanks, @sarnex. I added a few questions/comments, but don't have any change requests at present.

[tahonermann]

No need to specify a priority for this call?

It seems it is never (supposed to be) possible for both CodeGenOpts.OffloadBinaryToEmbedFile.empty() and CodeGenOpts.OffloadObjects.empty() to be false. Perhaps it is worth adding an assert to the OffloadBinaryToEmbedFile block to ensure that CodeGenOpts.OffloadObjects.empty() is true?

[tahonermann]

Should there be some kind of check that a prefix was provided by -fsycl-unique-prefix=? A prefix might not be provided when separate host and device compilations are performed (I don't recall if we support that).

[tahonermann]

Why are these declarations present in the test?