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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
