[PATCH] D145770: [clang-offload-bundler] Standardize TargetID field for bundler

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D145770#4185658 <https://reviews.llvm.org/D145770#4185658>, @lamb-j wrote:

> In D145770#4184996 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145770/new/

https://reviews.llvm.org/D145770