[OffloadBundler] Rework the ctor of `OffloadTargetInfo` to support AMDGPU's generic target #122629

shiltian · 2025-01-12T04:23:01Z

The current parsing logic for the target string assumes it follows the format <kind>-<triple>-<target id>:<feature>, such as hipv4-amdgcn-amd-amdhsa-gfx1030:+xnack.
Specifically, it assumes that <target id> does not contain any -, relying on rsplit for parsing.
However, this assumption breaks for AMDGPU's generic targets, which may contain one or more -, such as gfx10-3-generic or gfx12-generic.
As a result, the existing approach using rstrip is no longer reliable.

This patch reworks the parsing logic to handle target strings more robustly, including support for generic targets.
The bundler now strictly requires a 4-field target triple.
Additionally, a new Python helper function has been added to config.py to normalize the target triple into the 4-field format when it is not, ensuring tests pass reliably.

shiltian · 2025-01-12T04:23:21Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

llvmbot · 2025-01-12T04:23:34Z

@llvm/pr-subscribers-testing-tools
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Shilei Tian (shiltian)

Changes

The current parsing of target string assumes to be in a form of
kind-triple-targetid:feature, such as
hipv4-amdgcn-amd-amdhsa-gfx1030:+xnack. Specifically, the target id does not
contain any -, which is not the case for generic target. Also, a generic
target may contain one or more -, such as gfx10-3-generic and
gfx12-generic. As a result, we can no longer depend on rstrip to get things
work right. This patch reworks the logic to parse the target string to make it
more robust, as well as supporting generic target.

Full diff: https://github.com/llvm/llvm-project/pull/122629.diff

6 Files Affected:

(modified) clang/docs/ClangOffloadBundler.rst (+3-4)
(modified) clang/lib/Driver/OffloadBundler.cpp (+14-26)
(modified) clang/test/Driver/clang-offload-bundler-asserts-on.c (+7-7)
(modified) clang/test/Driver/clang-offload-bundler-standardize.c (+6-7)
(modified) clang/test/Driver/clang-offload-bundler-zstd.c (+6-6)
(modified) clang/test/Driver/clang-offload-bundler.c (+10-10)

diff --git a/clang/docs/ClangOffloadBundler.rst b/clang/docs/ClangOffloadBundler.rst
index 3c241027d405ca..25214c2ea6a4e1 100644
--- a/clang/docs/ClangOffloadBundler.rst
+++ b/clang/docs/ClangOffloadBundler.rst
@@ -266,15 +266,14 @@ without differentiation based on offload kind.
     The target triple of the code object. See `Target Triple
     <https://clang.llvm.org/docs/CrossCompilation.html#target-triple>`_.
 
-    The bundler accepts target triples with or without the optional environment
-    field:
+    LLVM target triples can be with or without the optional environment field:
 
     ``<arch><sub>-<vendor>-<sys>``, or
     ``<arch><sub>-<vendor>-<sys>-<env>``
 
     However, in order to standardize outputs for tools that consume bitcode
-    bundles, bundles written by the bundler internally use only the 4-field
-    target triple:
+    bundles, the bundler only accepts target triples with the 4-field target
+    triple:
 
     ``<arch><sub>-<vendor>-<sys>-<env>``
 
diff --git a/clang/lib/Driver/OffloadBundler.cpp b/clang/lib/Driver/OffloadBundler.cpp
index 2d6bdff0393be5..474b5a0fa4148f 100644
--- a/clang/lib/Driver/OffloadBundler.cpp
+++ b/clang/lib/Driver/OffloadBundler.cpp
@@ -84,31 +84,19 @@ OffloadTargetInfo::OffloadTargetInfo(const StringRef Target,
     : BundlerConfig(BC) {
 
   // TODO: Add error checking from ClangOffloadBundler.cpp
-  auto TargetFeatures = Target.split(':');
-  auto TripleOrGPU = TargetFeatures.first.rsplit('-');
-
-  if (clang::StringToOffloadArch(TripleOrGPU.second) !=
-      clang::OffloadArch::UNKNOWN) {
-    auto KindTriple = TripleOrGPU.first.split('-');
-    this->OffloadKind = KindTriple.first;
-
-    // Enforce optional env 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;
-
-    // Enforce optional env 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 = "";
-  }
+  // <kind>-<triple>[-<target id>]
+  // <tiple> := <arch>-<vendor>-<os>-<env>
+  SmallVector<StringRef, 6> Components;
+  Target.split(':').first.split(Components, '-', /*MaxSplit=*/5);
+  assert((Components.size() == 5 || Components.size() == 6) &&
+         "malformed target string");
+
+  this->OffloadKind = Components.front();
+  this->TargetID = Components.size() == 6 ? Components.back() : "";
+  ArrayRef<StringRef> TripleSlice{&Components[1], /*length=*/4};
+  llvm::Triple T = llvm::Triple(llvm::join(TripleSlice, "-"));
+  this->Triple = llvm::Triple(T.getArchName(), T.getVendorName(), T.getOSName(),
+                              T.getEnvironmentName());
 }
 
 bool OffloadTargetInfo::hasHostKind() const {
@@ -148,7 +136,7 @@ bool OffloadTargetInfo::operator==(const OffloadTargetInfo &Target) const {
 }
 
 std::string OffloadTargetInfo::str() const {
-  return Twine(OffloadKind + "-" + Triple.str() + "-" + TargetID).str();
+  return Twine(OffloadKind + "-" + Triple.normalize() + "-" + TargetID).str();
 }
 
 static StringRef getDeviceFileExtension(StringRef Device,
diff --git a/clang/test/Driver/clang-offload-bundler-asserts-on.c b/clang/test/Driver/clang-offload-bundler-asserts-on.c
index 55060c2c42e734..eec30674107b25 100644
--- a/clang/test/Driver/clang-offload-bundler-asserts-on.c
+++ b/clang/test/Driver/clang-offload-bundler-asserts-on.c
@@ -15,20 +15,20 @@
 // Check code object compatibility for archive unbundling
 //
 // 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--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--gfx906:sramecc+:xnack+,openmp-amdgcn-amd-amdhsa--gfx908:sramecc+:xnack+ -inputs=%t.o,%t.tgt1,%t.tgt1 -outputs=%t.targetID1.bundle
 // RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-amdgcn-amd-amdhsa--gfx906:sramecc+:xnack-,openmp-amdgcn-amd-amdhsa--gfx908:sramecc+:xnack- -inputs=%t.o,%t.tgt1,%t.tgt1 -outputs=%t.targetID2.bundle
 // RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,openmp-amdgcn-amd-amdhsa--gfx906:xnack-,openmp-amdgcn-amd-amdhsa--gfx908:xnack- -inputs=%t.o,%t.tgt1,%t.tgt1 -outputs=%t.targetID3.bundle
 // RUN: llvm-ar cr %t.input-archive.a %t.simple.bundle %t.targetID1.bundle %t.targetID2.bundle %t.targetID3.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]
+// 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-unknown-gfx906]  :       [Target: openmp-amdgcn-amd-amdhsa-unknown-gfx906]
+// BUNDLECOMPATIBILITY: Compatible: Exact match:        [CodeObject: openmp-amdgcn-amd-amdhsa-unknown-gfx908]  :       [Target: openmp-amdgcn-amd-amdhsa-unknown-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: Target IDs are compatible        [CodeObject: openmp-amdgcn-amd-amdhsa--gfx906]  :       [Target: hip-amdgcn-amd-amdhsa--gfx906]
-// HIPOpenMPCOMPATIBILITY: Compatible: Target IDs are compatible        [CodeObject: openmp-amdgcn-amd-amdhsa--gfx908]  :       [Target: hipv4-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: Target IDs are compatible        [CodeObject: openmp-amdgcn-amd-amdhsa-unknown-gfx906]  :       [Target: hip-amdgcn-amd-amdhsa-unknown-gfx906]
+// HIPOpenMPCOMPATIBILITY: Compatible: Target IDs are compatible        [CodeObject: openmp-amdgcn-amd-amdhsa-unknown-gfx908]  :       [Target: hipv4-amdgcn-amd-amdhsa-unknown-gfx908]
 
 // Some code so that we can create a binary out of this file.
 int A = 0;
diff --git a/clang/test/Driver/clang-offload-bundler-standardize.c b/clang/test/Driver/clang-offload-bundler-standardize.c
index 52f5ea038e47b8..6550b8051987ab 100644
--- a/clang/test/Driver/clang-offload-bundler-standardize.c
+++ b/clang/test/Driver/clang-offload-bundler-standardize.c
@@ -16,19 +16,18 @@
 // Check code object compatibility for archive unbundling
 //
 // Create an object bundle with and without env fields
-// RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,hip-amdgcn-amd-amdhsa-gfx906,hip-amdgcn-amd-amdhsa-gfx908 -input=%t.o -input=%t.tgt1 -input=%t.tgt2 -output=%t.bundle.no.env
-// RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple-,hip-amdgcn-amd-amdhsa--gfx906,hip-amdgcn-amd-amdhsa--gfx908 -input=%t.o -input=%t.tgt1 -input=%t.tgt2 -output=%t.bundle.env
+// RUN: clang-offload-bundler -type=o -targets=host-%itanium_abi_triple,hip-amdgcn-amd-amdhsa--gfx906,hip-amdgcn-amd-amdhsa--gfx908 -input=%t.o -input=%t.tgt1 -input=%t.tgt2 -output=%t.bundle.no.env
 
 
 // Unbundle bundle.no.env while providing targets with env
 // RUN: clang-offload-bundler -unbundle -type=o -targets=hip-amdgcn-amd-amdhsa--gfx906,hip-amdgcn-amd-amdhsa--gfx908 -input=%t.bundle.no.env -output=%t-hip-amdgcn-amd-amdhsa--gfx906.bc -output=%t-hip-amdgcn-amd-amdhsa--gfx908.bc -debug-only=CodeObjectCompatibility 2>&1 | FileCheck %s -check-prefix=BUNDLE-NO-ENV
-// BUNDLE-NO-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa--gfx906] : [Target: hip-amdgcn-amd-amdhsa--gfx906]
-// BUNDLE-NO-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa--gfx908] : [Target: hip-amdgcn-amd-amdhsa--gfx908]
+// BUNDLE-NO-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa-unknown-gfx906] : [Target: hip-amdgcn-amd-amdhsa-unknown-gfx906]
+// BUNDLE-NO-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa-unknown-gfx908] : [Target: hip-amdgcn-amd-amdhsa-unknown-gfx908]
 
 // Unbundle bundle.env while providing targets with no env
-// RUN: clang-offload-bundler -unbundle -type=o -targets=hip-amdgcn-amd-amdhsa-gfx906,hip-amdgcn-amd-amdhsa-gfx908 -input=%t.bundle.env -output=%t-hip-amdgcn-amd-amdhsa-gfx906.bc -output=%t-hip-amdgcn-amd-amdhsa-gfx908.bc -debug-only=CodeObjectCompatibility 2>&1 | FileCheck %s -check-prefix=BUNDLE-ENV
-// BUNDLE-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa--gfx906] : [Target: hip-amdgcn-amd-amdhsa--gfx906]
-// BUNDLE-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa--gfx908] : [Target: hip-amdgcn-amd-amdhsa--gfx908]
+// RUN: clang-offload-bundler -unbundle -type=o -targets=hip-amdgcn-amd-amdhsa--gfx906,hip-amdgcn-amd-amdhsa--gfx908 -input=%t.bundle.env -output=%t-hip-amdgcn-amd-amdhsa--gfx906.bc -output=%t-hip-amdgcn-amd-amdhsa--gfx908.bc -debug-only=CodeObjectCompatibility 2>&1 | FileCheck %s -check-prefix=BUNDLE-ENV
+// BUNDLE-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa-unknown-gfx906] : [Target: hip-amdgcn-amd-amdhsa-unknown-gfx906]
+// BUNDLE-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa-unknown-gfx908] : [Target: hip-amdgcn-amd-amdhsa-unknown-gfx908]
 
 // Some code so that we can create a binary out of this file.
 int A = 0;
diff --git a/clang/test/Driver/clang-offload-bundler-zstd.c b/clang/test/Driver/clang-offload-bundler-zstd.c
index 667d9554daec71..60521628a4a2ab 100644
--- a/clang/test/Driver/clang-offload-bundler-zstd.c
+++ b/clang/test/Driver/clang-offload-bundler-zstd.c
@@ -38,8 +38,8 @@
 // CHECK: Decompression method: zstd
 // CHECK: Hashes match: Yes
 // NOHOST-NOT: host-
-// NOHOST-DAG: hip-amdgcn-amd-amdhsa--gfx900
-// NOHOST-DAG: hip-amdgcn-amd-amdhsa--gfx906
+// NOHOST-DAG: hip-amdgcn-amd-amdhsa-unknown-gfx900
+// NOHOST-DAG: hip-amdgcn-amd-amdhsa-unknown-gfx906
 //
 
 // Check -compression-level= option
@@ -77,10 +77,10 @@
 // RUN:   -output=%t.hip_900.a -output=%t.hip_906.a -input=%t.hip_archive.a
 // RUN: llvm-ar t %t.hip_900.a | FileCheck -check-prefix=HIP-AR-900 %s
 // RUN: llvm-ar t %t.hip_906.a | FileCheck -check-prefix=HIP-AR-906 %s
-// HIP-AR-900-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa--gfx900
-// HIP-AR-900-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa--gfx900
-// HIP-AR-906-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa--gfx906
-// HIP-AR-906-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa--gfx906
+// HIP-AR-900-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa-unknown-gfx900
+// HIP-AR-900-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa-unknown-gfx900
+// HIP-AR-906-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa-unknown-gfx906
+// HIP-AR-906-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa-unknown-gfx906
 
 // Some code so that we can create a binary out of this file.
 int A = 0;
diff --git a/clang/test/Driver/clang-offload-bundler.c b/clang/test/Driver/clang-offload-bundler.c
index 1909ff2d71d03c..8757beab21beea 100644
--- a/clang/test/Driver/clang-offload-bundler.c
+++ b/clang/test/Driver/clang-offload-bundler.c
@@ -116,7 +116,7 @@
 // RUN: not clang-offload-bundler -type=i -targets=host-powerpc64le-ibm-linux-gnu,openmp-powerpc64le-ibm-linux-gnu,xpenmp-x86_xx-pc-linux-gnu -input=%t.i -input=%t.tgt1 -input=%t.tgt2 -output=%t.bundle.i 2>&1 | FileCheck %s --check-prefix CK-ERR8B
 // CK-ERR8B: error: invalid target 'xpenmp-x86_xx-pc-linux-gnu', unknown offloading kind 'xpenmp', unknown target triple 'x86_xx-pc-linux-gnu'
 
-// RUN: not clang-offload-bundler -type=i -targets=openmp-powerpc64le-linux,openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -input=%t.i -input=%t.tgt1 -input=%t.tgt2 -output=%t.bundle.i 2>&1 | FileCheck %s --check-prefix CK-ERR9A
+// RUN: not clang-offload-bundler -type=i -targets=openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu -input=%t.i -input=%t.tgt1 -output=%t.bundle.i 2>&1 | FileCheck %s --check-prefix CK-ERR9A
 // CK-ERR9A: error: expecting exactly one host target but got 0
 
 // RUN: not clang-offload-bundler -type=i -targets=host-%itanium_abi_triple,host-%itanium_abi_triple,openmp-x86_64-pc-linux-gnu -input=%t.i -input=%t.tgt1 -input=%t.tgt2 -output=%t.bundle.i 2>&1 | FileCheck %s --check-prefix CK-ERR9B
@@ -364,15 +364,15 @@
 // RUN: not clang-offload-bundler -type=bc -input=%t.hip.bundle.bc -output=%t.tmp.bc -output=%t.tmp2.bc -unbundle \
 // RUN:   -targets=hip-amdgcn-amd-amdhsa--gfx906,hip-amdgcn-amd-amdhsa--gfx900 \
 // RUN:   2>&1 | FileCheck -check-prefix=MISS1 %s
-// MISS1: error: Can't find bundles for hip-amdgcn-amd-amdhsa--gfx906
+// MISS1: error: Can't find bundles for hip-amdgcn-amd-amdhsa-unknown-gfx906
 // RUN: not clang-offload-bundler -type=bc -input=%t.hip.bundle.bc -output=%t.tmp.bc -output=%t.tmp2.bc -unbundle \
 // RUN:   -targets=hip-amdgcn-amd-amdhsa--gfx906,hip-amdgcn-amd-amdhsa--gfx803 \
 // RUN:   2>&1 | FileCheck -check-prefix=MISS2 %s
-// MISS2: error: Can't find bundles for hip-amdgcn-amd-amdhsa--gfx803 and hip-amdgcn-amd-amdhsa--gfx906
+// MISS2: error: Can't find bundles for hip-amdgcn-amd-amdhsa-unknown-gfx803 and hip-amdgcn-amd-amdhsa-unknown-gfx906
 // RUN: not clang-offload-bundler -type=bc -input=%t.hip.bundle.bc -output=%t.tmp.bc -output=%t.tmp2.bc -output=%t.tmp3.bc -unbundle \
 // RUN:   -targets=hip-amdgcn-amd-amdhsa--gfx906,hip-amdgcn-amd-amdhsa--gfx803,hip-amdgcn-amd-amdhsa--gfx1010 \
 // RUN:   2>&1 | FileCheck -check-prefix=MISS3 %s
-// MISS3: error: Can't find bundles for hip-amdgcn-amd-amdhsa--gfx1010, hip-amdgcn-amd-amdhsa--gfx803, and hip-amdgcn-amd-amdhsa--gfx906
+// MISS3: error: Can't find bundles for hip-amdgcn-amd-amdhsa-unknown-gfx1010, hip-amdgcn-amd-amdhsa-unknown-gfx803, and hip-amdgcn-amd-amdhsa-unknown-gfx906
 
 //
 // Check error due to duplicate targets
@@ -422,10 +422,10 @@
 // RUN:   -output=%T/hip_900.a -output=%T/hip_906.a -input=%T/hip_archive.a
 // RUN: llvm-ar t %T/hip_900.a | FileCheck -check-prefix=HIP-AR-900 %s
 // RUN: llvm-ar t %T/hip_906.a | FileCheck -check-prefix=HIP-AR-906 %s
-// HIP-AR-900-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa--gfx900
-// HIP-AR-900-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa--gfx900
-// HIP-AR-906-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa--gfx906
-// HIP-AR-906-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa--gfx906
+// HIP-AR-900-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa-unknown-gfx900
+// HIP-AR-900-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa-unknown-gfx900
+// HIP-AR-906-DAG: hip_bundle1-hip-amdgcn-amd-amdhsa-unknown-gfx906
+// HIP-AR-906-DAG: hip_bundle2-hip-amdgcn-amd-amdhsa-unknown-gfx906
 
 //
 // Check unbundling archive for host target
@@ -469,8 +469,8 @@
 // RUN: diff %t.tgt2 %t.res.tgt2
 //
 // NOHOST-NOT: host-
-// NOHOST-DAG: hip-amdgcn-amd-amdhsa--gfx900
-// NOHOST-DAG: hip-amdgcn-amd-amdhsa--gfx906
+// NOHOST-DAG: hip-amdgcn-amd-amdhsa-unknown-gfx900
+// NOHOST-DAG: hip-amdgcn-amd-amdhsa-unknown-gfx906
 
 //
 // Check bundling ID compatibility for HIP.

clang/lib/Driver/OffloadBundler.cpp

shiltian · 2025-01-12T04:28:32Z

I suppose this change is quite intrusive for any external use of clang-offload-bundler. It's gonna require all the targets listed in -targets have to be in the normalized form (4-field).

yxsamliu · 2025-01-13T14:16:58Z

Did you try this patch with internal PSDB? Does it break any existing HIP apps or libraries?

shiltian · 2025-01-13T16:02:40Z

Did you try this patch with internal PSDB? Does it break any existing HIP apps or libraries?

Will make a PR shortly after the migration.

lamb-j · 2025-01-13T17:34:14Z

Is there a way for us to still support 3-field triples and generic targets?

In the past we worked to allow 3-field triples as bundler inputs. I would be surprised if adding a 4-field restriction didn't break something internally.

shiltian · 2025-01-13T17:39:01Z

Is there a way for us to still support 3-field triples and generic targets?

I can't come up with a better solution because we don't know how many - could exist in the (generic) target id. Based on current situation, it could be 0, 1, and 2. With that, fixating the "known" part (aka triple) would be reasonable. Do you have any suggestions?

An alternative would be to change the syntax of target string to <kind>:<triple>:<target id>:<target features>. There is no ambiguity in this way, but I'd assume that is a more intrusive change.

In the past we worked to allow 3-field triples as bundler inputs. I would be surprised if adding a 4-field restriction didn't break something internally.

I don't know if there is any project using clang-offload-bundler directly. If there is, and we decide to enforce it, we will have to update them as well.

lamb-j · 2025-01-13T18:05:39Z

https://github.com/search?q=org%3AROCm%20clang-offload-bundler&type=code

So at least a few direct users this may break things for. There are also users that call the OffloadBundler API instead of directly calling clang-offload-bundler that this may impact.

Could we assume "gfx * -generic" is a single Target ID, and check for/parse that separately before doing rsplit on "-"?

shiltian · 2025-01-13T18:21:26Z

Could we assume "gfx * -generic" is a single Target ID, and check for/parse that separately before doing rsplit on "-"?

I was thinking about it the other day but eventually gave up on it, because I want to keep this part as generic as possible instead of adding AMD special sauce here. I prefer not to do something like pattern matching stuff about gfx.

shiltian · 2025-01-13T18:25:05Z

So at least a few direct users this may break things for. There are also users that call the OffloadBundler API instead of directly calling clang-offload-bundler that this may impact.

For the API usage there will be no issue because we enforce it in the code already. The only part needs to be updated is those binary user.

lamb-j · 2025-01-17T06:57:22Z

Could we possibly do a phase-out approach?

For COV5 and earlier we keep the current strategy, allowing both 3 and 4 field triples

For COV6 (including generics), we enforce the 4 field triples

This could help us not break things internally, and then as tools and APIs update to V6, they can also update to 4-field triples

shiltian · 2025-01-17T16:06:34Z

Isn't offload bundler a target agnostic tool?

This could help us not break things internally, and then as tools and APIs update to V6, they can also update to 4-field triples

PSDB has full green (though some tests were skipped due to PSDB issue but I understand this doesn't mean we wouldn't break anything).

I also created issues to all repos that use clang-offload-bundler binary directly. Most of them have responded and started using 4-tuple now. IMHO if we tell users, hey, if you do COV5, you can use 3- or 4-tuple, but if you do COV6, you have to use 4-tuple, it's gonna me way more confusing than directly enforcing them. In addition, internally we have switched to COV6 by default already for staging branch, where this PR is gonna go, and will be soon in mainline.

jhuber6 · 2025-02-10T16:21:50Z

Isn't offload bundler a target agnostic tool?

Would it break anything if we just assumed it was always amdgcn-amd-amdhsa for the triple? It's supposed to be generic, but right now only HIP uses it AFAICT.

clang/docs/ClangOffloadBundler.rst

clang/lib/Driver/OffloadBundler.cpp

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

…neric target The current parsing of target string assumes to be in a form of `kind-triple-targetid:feature`, such as `hipv4-amdgcn-amd-amdhsa-gfx1030:+xnack`. Specifically, the target id does not contain any `-`, which is not the case for generic target. Also, a generic target may contain one or more `-`, such as `gfx10-3-generic` and `gfx12-generic`. As a result, we can no longer depend on `rstrip` to get things work right. This patch reworks the logic to parse the target string to make it more robust, as well as supporting generic target.

…t bundle id

yxsamliu

LGTM. Thanks

shiltian · 2025-03-18T16:21:22Z

It seems like some targets add "-unknown" in the compiler driver.

+ /local/mnt/workspace/bots/hexagon-build-02/clang-hexagon-elf/stage1/bin/clang --target=hexagon-unknown-elf-unknown -fverbose-asm -g -S /local/mnt/workspace/bots/hexagon-build-02/clang-hexagon-elf/llvm/clang/test/CodeGenCXX/debug-info-member.cpp -o -
clang: error: version 'elf-unknown' in target triple 'hexagon-unknown-unknown-elf-unknown' is invalid

+ /home/buildbot-worker/bbroot/clang-riscv-rva20-2stage/stage2/bin/clang --target=riscv64-linux-gnu-unknown -g -S /home/buildbot-worker/bbroot/clang-riscv-rva20-2stage/llvm/clang/test/CodeGenCXX/debug-info-byval.cpp -o -
clang: error: version '-unknown' in target triple 'riscv64-unknown-linux-gnu-unknown' is invalid
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/buildbot-worker/bbroot/clang-riscv-rva20-2stage/stage2/bin/FileCheck /home/buildbot-worker/bbroot/clang-riscv-rva20-2stage/llvm/clang/test/CodeGenCXX/debug-info-byval.cpp

…pport AMDGPU's generic target (llvm#122629)" This reverts commit d85a81b.

pratlucas · 2025-03-20T11:24:47Z

I see this change re-landed yesterday but we continue to see failures related to the extra -unknown component in our downstream testing of arm/arm-toolchain, which are similar to what was described in the comment above:

arm-software/build-atfe-package/clang/test/Driver/debug-options-as.c:88:12: error: DWARF3: expected string not found in input
// DWARF3: "-cc1as"
           ^
<stdin>:1:1: note: scanning from here
clang: error: version '-unknown' in target triple 'aarch64-unknown-linux-gnu-unknown' is invalid
^

shiltian · 2025-03-20T13:18:32Z

Yeah, that needs to be fixed. The command line argument is --target=hexagon-unknown-elf-unknown, which is a valid target triple, but the error is hexagon-unknown-unknown-elf-unknown. That definitely exposes the issue in the compiler driver.

…DGPU's generic target (llvm#122629) The current parsing logic for the target string assumes it follows the format `<kind>-<triple>-<target id>:<feature>`, such as `hipv4-amdgcn-amd-amdhsa-gfx1030:+xnack`. Specifically, it assumes that `<target id>` does not contain any `-`, relying on `rsplit` for parsing. However, this assumption breaks for AMDGPU's generic targets, which may contain one or more `-`, such as `gfx10-3-generic` or `gfx12-generic`. As a result, the existing approach using `rstrip` is no longer reliable. This patch reworks the parsing logic to handle target strings more robustly, including support for generic targets. The bundler now strictly requires a 4-field target triple. Additionally, a new Python helper function has been added to `config.py` to normalize the target triple into the 4-field format when it is not, ensuring tests pass reliably. (cherry picked from commit d85a81b)

…upport AMDGPU's generic target (llvm#122629)" This reverts commit 9c870cb.

asb · 2025-04-01T20:03:01Z

It seems like some targets add "-unknown" in the compiler driver.

+ /local/mnt/workspace/bots/hexagon-build-02/clang-hexagon-elf/stage1/bin/clang --target=hexagon-unknown-elf-unknown -fverbose-asm -g -S /local/mnt/workspace/bots/hexagon-build-02/clang-hexagon-elf/llvm/clang/test/CodeGenCXX/debug-info-member.cpp -o -
clang: error: version 'elf-unknown' in target triple 'hexagon-unknown-unknown-elf-unknown' is invalid

+ /home/buildbot-worker/bbroot/clang-riscv-rva20-2stage/stage2/bin/clang --target=riscv64-linux-gnu-unknown -g -S /home/buildbot-worker/bbroot/clang-riscv-rva20-2stage/llvm/clang/test/CodeGenCXX/debug-info-byval.cpp -o -
clang: error: version '-unknown' in target triple 'riscv64-unknown-linux-gnu-unknown' is invalid
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/buildbot-worker/bbroot/clang-riscv-rva20-2stage/stage2/bin/FileCheck /home/buildbot-worker/bbroot/clang-riscv-rva20-2stage/llvm/clang/test/CodeGenCXX/debug-info-byval.cpp

This is breaking multiple buildbots, so the correct fix is to revert and rework. I have erred in not spotting this breakage earlier as I incorrectly believed I'd receive an email as bot owner for a new failure. The correct path forwards should have been a revert https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy though I know time has passed now.

@pratlucas did you get anywhere looking into your downstream breakage? CC @SundeepKushwaha on the hexagon side. I'll take a closer look tomorrow.

pratlucas · 2025-04-02T15:45:31Z

Thanks for picking this up @asb ! Unfortunately I still couldn't find the time to investigate the root cause of the issue in our downstream runs. I'll keep you posted in case we find any more details on this.

shiltian · 2025-04-02T15:52:22Z

@asb I did receive some emails initially but haven't gotten any lately. I assumed that issue had been fixed, as it seemed like the driver was just unconditionally adding unknown.

…DGPU's generic target (llvm#122629) The current parsing logic for the target string assumes it follows the format `<kind>-<triple>-<target id>:<feature>`, such as `hipv4-amdgcn-amd-amdhsa-gfx1030:+xnack`. Specifically, it assumes that `<target id>` does not contain any `-`, relying on `rsplit` for parsing. However, this assumption breaks for AMDGPU's generic targets, which may contain one or more `-`, such as `gfx10-3-generic` or `gfx12-generic`. As a result, the existing approach using `rstrip` is no longer reliable. This patch reworks the parsing logic to handle target strings more robustly, including support for generic targets. The bundler now strictly requires a 4-field target triple. Additionally, a new Python helper function has been added to `config.py` to normalize the target triple into the 4-field format when it is not, ensuring tests pass reliably. (cherry picked from commit d85a81b)

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jan 12, 2025

shiltian mentioned this pull request Jan 12, 2025

[Clang] Use -targets=host-x86_64-unknown-linux-gnu as bundler target #122627

Merged

shiltian requested review from jchlanda, yxsamliu, jhuber6 and lamb-j January 12, 2025 04:23

shiltian commented Jan 12, 2025

View reviewed changes

clang/lib/Driver/OffloadBundler.cpp Outdated Show resolved Hide resolved

shiltian force-pushed the users/shiltian/triple-with-environment branch from 95332e0 to 71bad77 Compare January 12, 2025 15:49

shiltian force-pushed the users/shiltian/rework-offloadtargetinfo-ctor branch from 2fc9d92 to 1b69731 Compare January 12, 2025 15:49

llvmbot added llvm-lit testing-tools labels Jan 12, 2025

Base automatically changed from users/shiltian/triple-with-environment to main January 12, 2025 15:51

shiltian force-pushed the users/shiltian/rework-offloadtargetinfo-ctor branch 2 times, most recently from b1dbe4f to c58bbd1 Compare January 12, 2025 15:52

shiltian changed the title ~~[OffloadBundler] Rework the ctor of OffloadTargetInfo to support generic target~~ [OffloadBundler] Rework the ctor of OffloadTargetInfo to support AMDGPU's generic target Jan 12, 2025

shiltian force-pushed the users/shiltian/rework-offloadtargetinfo-ctor branch 2 times, most recently from 2fdbbac to 26ebaf0 Compare January 12, 2025 23:02

shiltian force-pushed the users/shiltian/rework-offloadtargetinfo-ctor branch from 26ebaf0 to f5e2dd6 Compare January 14, 2025 16:59

shiltian changed the base branch from main to users/shiltian/normalize-with-num-fields January 14, 2025 16:59

shiltian force-pushed the users/shiltian/rework-offloadtargetinfo-ctor branch from 1b13d14 to 0075687 Compare February 10, 2025 16:19

shiltian force-pushed the users/shiltian/rework-offloadtargetinfo-ctor branch from 0075687 to 84b10d4 Compare February 15, 2025 00:13

shiltian force-pushed the users/shiltian/rework-offloadtargetinfo-ctor branch from 84b10d4 to 1650912 Compare March 17, 2025 16:31

yxsamliu reviewed Mar 17, 2025

View reviewed changes

clang/docs/ClangOffloadBundler.rst Outdated Show resolved Hide resolved

yxsamliu reviewed Mar 17, 2025

View reviewed changes

clang/lib/Driver/OffloadBundler.cpp Show resolved Hide resolved

yxsamliu reviewed Mar 17, 2025

View reviewed changes

clang/test/Driver/clang-offload-bundler.c Show resolved Hide resolved

shiltian force-pushed the users/shiltian/rework-offloadtargetinfo-ctor branch from 8df4ce5 to 44bcda0 Compare March 17, 2025 17:40

shiltian added 4 commits March 17, 2025 17:43

Update document

099faed

Add bundle id check such as it should not crash when given a malforma…

8c59de8

…t bundle id

Fix a test issue

29c3cac

shiltian force-pushed the users/shiltian/rework-offloadtargetinfo-ctor branch from 44bcda0 to 29c3cac Compare March 17, 2025 21:47

yxsamliu approved these changes Mar 18, 2025

View reviewed changes

shiltian merged commit d85a81b into main Mar 18, 2025
12 checks passed

shiltian deleted the users/shiltian/rework-offloadtargetinfo-ctor branch March 18, 2025 14:09

searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Mar 19, 2025

Revert "[OffloadBundler] Rework the ctor of OffloadTargetInfo to su…

9c870cb

…pport AMDGPU's generic target (llvm#122629)" This reverts commit d85a81b.

searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Mar 29, 2025

Reapply "[OffloadBundler] Rework the ctor of OffloadTargetInfo to s…

744d4ad

…upport AMDGPU's generic target (llvm#122629)" This reverts commit 9c870cb.

UsmanNadeem mentioned this pull request Apr 13, 2025

[LLVM][Triple] Drop unknown object types from normalized triples #135571

Open

UsmanNadeem mentioned this pull request May 6, 2025

Fix buildbot failure due to triple normalization issues #138747

Draft

[OffloadBundler] Rework the ctor of OffloadTargetInfo to support AMDGPU's generic target #122629

[OffloadBundler] Rework the ctor of OffloadTargetInfo to support AMDGPU's generic target #122629

Uh oh!

Conversation

shiltian commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiltian commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shiltian commented Jan 12, 2025

Uh oh!

yxsamliu commented Jan 13, 2025

Uh oh!

shiltian commented Jan 13, 2025

Uh oh!

lamb-j commented Jan 13, 2025

Uh oh!

shiltian commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lamb-j commented Jan 13, 2025

Uh oh!

shiltian commented Jan 13, 2025

Uh oh!

shiltian commented Jan 13, 2025

Uh oh!

lamb-j commented Jan 17, 2025

Uh oh!

shiltian commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhuber6 commented Feb 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yxsamliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shiltian commented Mar 18, 2025

Uh oh!

pratlucas commented Mar 20, 2025

Uh oh!

shiltian commented Mar 20, 2025

Uh oh!

asb commented Apr 1, 2025

Uh oh!

pratlucas commented Apr 2, 2025

Uh oh!

shiltian commented Apr 2, 2025

Uh oh!

Uh oh!

[OffloadBundler] Rework the ctor of `OffloadTargetInfo` to support AMDGPU's generic target #122629

[OffloadBundler] Rework the ctor of `OffloadTargetInfo` to support AMDGPU's generic target #122629

shiltian commented Jan 12, 2025 •

edited

Loading

shiltian commented Jan 12, 2025 •

edited

Loading

llvmbot commented Jan 12, 2025 •

edited

Loading

shiltian commented Jan 13, 2025 •

edited

Loading

shiltian commented Jan 17, 2025 •

edited

Loading