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

[Jacob Lambert via Phabricator via cfe-commits]

lamb-j added a comment.

In D145770#4186142 <https://reviews.llvm.org/D145770#4186142>, @yaxunl wrote:

> 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.

It may be fine to use "unknown" instead of an empty string, I didn't test that. It's just that the Triple.normalize() doesn't add anything for the env field if there wasn't anything there to begin with. That is, it doesn't convert a 3-field triple to a 4-field triple (it only converts a 4-field with empty string env to 4-field "unknown" env").

And sounds good about abi vs. env, maybe I'll add another patch to update CrossCompilation.html so it's more consistent with Triple.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145770