Is void amdgpu::Linker::ConstructJob() constructing a job for the host linker or the device linker? Or both?

In D145770#4220246, @mdtoguchi wrote:

@lamb-j - is it expected for any bundled objects created before your change without the explicit env field to be able to be unbundled? Newly generated bundles work as expected given similar -target values, but older generated binaries fail to unbundle the target given equivalent commands. Is it possible to provide the ability to do so?

Needed to update this after landing with two quick patch fixes:

Thanks for addressing this! I've just uploaded a patch that hopefully addresses the issue. If not I'll toss the XFAIL back on for ps4/5 and keep investigating.

Updating commit message

In D145770#4190901, @scott.linder wrote:

Is there a corresponding document update to go along with this? If consumers of the bundles will begin to rely on this it should be documented that it is guaranteed by the producer.

Adding note to documentation

Adding another test, updating existing large test, and switching from ABI
to env

In D145770#4186142, @yaxunl wrote:

In D145770#4185658, @lamb-j wrote:

In D145770#4184996, @yaxunl wrote:

The description needs fix. "ABI field" should be "environment component".

Also, we need a clang-offload-bundler test which bundles with a non-canonical triple and unbundles with a canonical triple and vice versa.

I'm not sure if we can use Triple.normalize(). Here's what I got from a small test:

amdgcn-amd-amdhsa  ->  amdgcn-amd-amdhsa (no env field added)
amdgcn-amd-amdhsa- ->  amdgcn-amd-amdhsa-unknown (empty env changed to unknown)

Also are we sure ABI is incorrect? That's what is used here:

https://clang.llvm.org/docs/CrossCompilation.html

(I'm happy to change to environment, just double checking)

https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/TargetParser/Triple.h only talks about "environment", which seems to be more accurate and up-to-date.

It is unfortunate that we have used our own normalization (empty string for unspecified environment instead of "unknown") for triple. Looks like we have to keep that for now. Ideally, HIP runtime should accept "amdgcn-amd-amdhsa-unknown" in addition to "amdgcn-amd-amdhsa--" in fat binary, then we could switch to the LLVM Triple normalization. But that requires runtime change first. Probably we could do that in future.

In D145770#4184996, @yaxunl wrote:

The description needs fix. "ABI field" should be "environment component".

Also, we need a clang-offload-bundler test which bundles with a non-canonical triple and unbundles with a canonical triple and vice versa.

We're ok with env field being blank, so this doesn't accomplish what we need

@thakis thanks for reverting that, sorry I didn't see your request sooner!

Updating driver cmake to correctly build shared library

Removing trailing whitespace

Fixing header typo

Switching pointers to references, adding namespace comment

Bug fix

Moving classes into clang namespace, more specific name for config class

Abandoned in favor of combined patch: https://reviews.llvm.org/D129873

Abandoned in favor of combined patch: https://reviews.llvm.org/D129873

Abandoned in favor of combined patch: https://reviews.llvm.org/D129873

Abandoned in favor of combined patch: https://reviews.llvm.org/D129873

@yaxunl Are you recommending I combine all 4 patches down into 1 patch? Or combine a subset of patches?

Adding clang-format

Adding clang-format

Adding clang-format

Switching test

Adding test

Updating commit message

Addressing clang-format comments

Address clang-format comment

Updating commit message

Switching to use new getTotalNumVGPRs from AMDGPUBaseInfo

Updating commit message

Fix small formatting error

Removing llvm namespace from function call and addressing clang-format issues