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

[lamb-j]

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.

[yaxunl]

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

[lamb-j]

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)

[yaxunl]

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.

[lamb-j]

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.

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]

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

[lamb-j]

clang/test/Driver/clang-offload-bundler.c

2 ↗

(On Diff #504310)

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.

[scott.linder]

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]

Adding note to documentation

[lamb-j]

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.

Good point, documentation udpated

[yaxunl]

LGTM. Please update the description.

[lamb-j]

Updating commit message

[dyung]

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

In D145770#4195018, @dyung wrote:

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

[lamb-j]

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)

[mdtoguchi]

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

[yaxunl]

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?

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

[lamb-j]

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?

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]

clang/lib/Driver/OffloadBundler.cpp

OffloadTargetInfo::OffloadTargetInfo(const StringRef Target,
 
   // TODO: Add error checking from ClangOffloadBundler.cpp
   auto TargetFeatures = Target.split(':');
   auto TripleOrGPU = TargetFeatures.first.rsplit('-');
 
   if (clang::StringToCudaArch(TripleOrGPU.second) != clang::CudaArch::UNKNOWN) {
     auto KindTriple = TripleOrGPU.first.split('-');
     this->OffloadKind = KindTriple.first;
-    this->Triple = llvm::Triple(KindTriple.second);
+
+    // Enforce optional ABI field to standardize bundles
+    llvm::Triple t = llvm::Triple(KindTriple.second);
+    this->Triple = llvm::Triple(t.getArchName(), t.getVendorName(),
+                                t.getOSName(), t.getEnvironmentName());
+
     this->TargetID = Target.substr(Target.find(TripleOrGPU.second));
   } else {
     auto KindTriple = TargetFeatures.first.split('-');
     this->OffloadKind = KindTriple.first;
-    this->Triple = llvm::Triple(KindTriple.second);
+
+    // Enforce optional ABI field to standardize bundles
+    llvm::Triple t = llvm::Triple(KindTriple.second);
+    this->Triple = llvm::Triple(t.getArchName(), t.getVendorName(),
+                                t.getOSName(), t.getEnvironmentName());
+
     this->TargetID = "";
   }
 }
 
 bool OffloadTargetInfo::hasHostKind() const {
   return this->OffloadKind == "host";
 }
 

clang/test/Driver/clang-offload-bundler-asserts-on.c

 //
 // Create few code object bundles and archive them to create an input archive
 // RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-amdgcn-amd-amdhsa-gfx906,openmp-amdgcn-amd-amdhsa--gfx908 -input=%t.o -input=%t.tgt1 -input=%t.tgt2 -output=%t.simple.bundle
 // RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-amdgcn-amd-amdhsa--gfx903 -input=%t.o -input=%t.tgt1 -output=%t.simple1.bundle
 // RUN: llvm-ar cr %t.input-archive.a %t.simple.bundle %t.simple1.bundle
 
 // Tests to check compatibility between Bundle Entry ID formats i.e. between presence/absence of extra hyphen in case of missing environment field
 // RUN: clang-offload-bundler -unbundle -type=a -targets=openmp-amdgcn-amd-amdhsa--gfx906,openmp-amdgcn-amd-amdhsa-gfx908 -input=%t.input-archive.a -output=%t-archive-gfx906-simple.a -output=%t-archive-gfx908-simple.a -debug-only=CodeObjectCompatibility 2>&1 | FileCheck %s -check-prefix=BUNDLECOMPATIBILITY
-// BUNDLECOMPATIBILITY: Compatible: Exact match:        [CodeObject: openmp-amdgcn-amd-amdhsa-gfx906]   :       [Target: openmp-amdgcn-amd-amdhsa--gfx906]
-// BUNDLECOMPATIBILITY: Compatible: Exact match:        [CodeObject: openmp-amdgcn-amd-amdhsa--gfx908]  :       [Target: openmp-amdgcn-amd-amdhsa-gfx908]
+// BUNDLECOMPATIBILITY: Compatible: Exact match:        [CodeObject: openmp-amdgcn-amd-amdhsa--gfx906]  :       [Target: openmp-amdgcn-amd-amdhsa--gfx906]
+// BUNDLECOMPATIBILITY: Compatible: Exact match:        [CodeObject: openmp-amdgcn-amd-amdhsa--gfx908]  :       [Target: openmp-amdgcn-amd-amdhsa--gfx908]
 
 // RUN: clang-offload-bundler -unbundle -type=a -targets=hip-amdgcn-amd-amdhsa--gfx906,hipv4-amdgcn-amd-amdhsa-gfx908 -input=%t.input-archive.a -output=%t-hip-archive-gfx906-simple.a -output=%t-hipv4-archive-gfx908-simple.a -hip-openmp-compatible -debug-only=CodeObjectCompatibility 2>&1 | FileCheck %s -check-prefix=HIPOpenMPCOMPATIBILITY
-// HIPOpenMPCOMPATIBILITY: Compatible: Code Objects are compatible        [CodeObject: openmp-amdgcn-amd-amdhsa-gfx906]   :       [Target: hip-amdgcn-amd-amdhsa--gfx906]
-// HIPOpenMPCOMPATIBILITY: Compatible: Code Objects are compatible        [CodeObject: openmp-amdgcn-amd-amdhsa--gfx908]  :       [Target: hipv4-amdgcn-amd-amdhsa-gfx908]
+// HIPOpenMPCOMPATIBILITY: Compatible: Code Objects are compatible        [CodeObject: openmp-amdgcn-amd-amdhsa--gfx906]  :       [Target: hip-amdgcn-amd-amdhsa--gfx906]
+// HIPOpenMPCOMPATIBILITY: Compatible: Code Objects are compatible        [CodeObject: openmp-amdgcn-amd-amdhsa--gfx908]  :       [Target: hipv4-amdgcn-amd-amdhsa--gfx908]
 
 // Some code so that we can create a binary out of this file.
 int A = 0;
 void test_func(void) {
   ++A;
 }

clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp

int main(int argc, const char **argv) {
   // Verify that the offload kinds and triples are known. We also check that we
   // have exactly one host target.
   unsigned Index = 0u;
   unsigned HostTargetNum = 0u;
   bool HIPOnly = true;
   llvm::DenseSet<StringRef> ParsedTargets;
   // Map {offload-kind}-{triple} to target IDs.
   std::map<std::string, std::set<StringRef>> TargetIDs;
+  // Standardize target names to include ABI field
+  std::vector<std::string> StandardizedTargetNames;
   for (StringRef Target : TargetNames) {
     if (ParsedTargets.contains(Target)) {
       reportError(createStringError(errc::invalid_argument,
                                     "Duplicate targets are not allowed"));
     }
     ParsedTargets.insert(Target);
 
     auto OffloadInfo = OffloadTargetInfo(Target, BundlerConfig);
     bool KindIsValid = OffloadInfo.isOffloadKindValid();
     bool TripleIsValid = OffloadInfo.isTripleValid();
 
+    StandardizedTargetNames.push_back(OffloadInfo.str());
+
     if (!KindIsValid || !TripleIsValid) {
       SmallVector<char, 128u> Buf;
       raw_svector_ostream Msg(Buf);
       Msg << "invalid target '" << Target << "'";
       if (!KindIsValid)
         Msg << ", unknown offloading kind '" << OffloadInfo.OffloadKind << "'";
       if (!TripleIsValid)
         Msg << ", unknown target triple '" << OffloadInfo.Triple.str() << "'";
       reportError(createStringError(errc::invalid_argument, Msg.str()));
     }
 
     TargetIDs[OffloadInfo.OffloadKind.str() + "-" + OffloadInfo.Triple.str()]
         .insert(OffloadInfo.TargetID);
     if (KindIsValid && OffloadInfo.hasHostKind()) {
       ++HostTargetNum;
       // Save the index of the input that refers to the host.
       BundlerConfig.HostInputIndex = Index;
     }
 
     if (OffloadInfo.OffloadKind != "hip" && OffloadInfo.OffloadKind != "hipv4")
       HIPOnly = false;
 
     ++Index;
   }
+
+  BundlerConfig.TargetNames = StandardizedTargetNames;
+
   for (const auto &TargetID : TargetIDs) {
     if (auto ConflictingTID =
             clang::getConflictTargetIDCombination(TargetID.second)) {
       SmallVector<char, 128u> Buf;
       raw_svector_ostream Msg(Buf);
       Msg << "Cannot bundle inputs with conflicting targets: '"
           << TargetID.first + "-" + ConflictingTID->first << "' and '"
           << TargetID.first + "-" + ConflictingTID->second << "'";