This is an archive of the discontinued LLVM Phabricator instance.

[clang-offload-bundler] Standardize TargetID field for bundler
ClosedPublic

Authored by lamb-j on Mar 10 2023, 1:03 AM.

Details

Summary

The bundler accepts both of the following for the --target option:

hip-amdgcn-amd-amdhsa-gfx900    (no ABI field)
hip-amdgcn-amd-amdhsa--gfx900   (blank ABI field)

The env (environment) field is defined as optional for Triples in Triple.h.
However, in this patch we update the bundler to internally
standardize to include the env field. While users aren't required
to specify an environment field when listing targets on the commandline,
bundles generated by the offload-bundler will include the env
field.

This standardization simplifies things for APIs that deal with
bundles generated by the clang-offload-bundler tool.

Diff Detail

Event Timeline

lamb-j created this revision.Mar 10 2023, 1:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2023, 1:03 AM
Herald added a subscriber: kosarev. · View Herald Transcript
lamb-j requested review of this revision.Mar 10 2023, 1:03 AM
Herald added a project: Restricted Project. · View Herald Transcript

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.

clang/lib/Driver/OffloadBundler.cpp
76–79

can we use Triple::normalize to normalize KindTriple.second, then use the returned string to create this->Triple? This is the conventional way to normalize the triple.

same as below

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)

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.

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

lamb-j updated this revision to Diff 504310.Mar 10 2023, 5:19 PM

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

lamb-j marked an inline comment as done.Mar 10 2023, 5:21 PM
lamb-j added inline comments.
clang/test/Driver/clang-offload-bundler.c
2

Can most systems cross-compile for powerpc? This test does a lot of testing, seems like it would be nice to test it on other systems.

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.

lamb-j updated this revision to Diff 504893.Mar 13 2023, 4:44 PM

Adding note to documentation

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.

Good point, documentation udpated

yaxunl accepted this revision.Mar 14 2023, 12:40 PM

LGTM. Please update the description.

This revision is now accepted and ready to land.Mar 14 2023, 12:40 PM
lamb-j updated this revision to Diff 505227.Mar 14 2023, 1:04 PM

Updating commit message

lamb-j edited the summary of this revision. (Show Details)Mar 14 2023, 1:06 PM
dyung added a subscriber: dyung.Mar 14 2023, 4:28 PM

@lamb-j the test clang-offload-bundler.c is failing on the PS4 linux builder, can you take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/37475

I don't know if it matters, but this bot only builds the x86_64 target.

dyung added a comment.Mar 14 2023, 6:36 PM

@lamb-j the test clang-offload-bundler.c is failing on the PS4 linux builder, can you take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/37475

I don't know if it matters, but this bot only builds the x86_64 target.

To unblock our bots, I've marked the test as XFAIL for PS4/PS5 in 768211f48f2d52fdd555a07f3dae8fbf1def88cc, please investigate when you can.

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

[clang-offload-bundler] Fix test failures and document typo (65fb636bd462)

[clang-offload-bundler] Fix error with regex in bundler test (f3b991202689)

There were also two quick fixes by others that overlapped but got in before my fixes:

Mark test modified in e48ae0d as XFAIL for PS4/PS5 until the author ... (768211f48f2d)

clang/test/Driver/clang-offload-bundler-standardize.c REQUIRES asserts. (fixup D145770) (d4a4d0d79127)

@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?

@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?

what is the target used by the old bundle? Thanks.

@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?

That should still be supported. The target triple for the old bundle should be converted to the new format (and compared against the Target-ID that was also converted to the new format). Can you provide an example to recreate the failure? I just tried one example locally and didn't hit any failures:

old-clang -c --offload-arch=gfx906 -emit-llvm -fgpu-rdc --gpu-bundle-output square.hip
new-clang-offload-bundler -unbundle -type=bc -targets=hip-amdgcn-amd-amdhsa-gfx906 -input=square.bc -output=square-hip-gfx906.bc -allow-missing-bundles -debug-only=CodeObjectCompatibility

Compatible: Exact match:        [CodeObject: hip-amdgcn-amd-amdhsa--gfx906]     :       [Target: hip-amdgcn-amd-amdhsa--gfx906]