Skip to content

Commit b701d28

Browse files
lamb-jshiltian
lamb-j
and
shiltian
authored
[OffloadBundler] Rework the ctor of OffloadTargetInfo to support AMDGPU's generic target
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. --------- Co-authored-by: Shilei Tian <[email protected]> Co-authored-by: Shilei Tian <[email protected]>
1 parent 36f33cb commit b701d28

18 files changed

+145
-98
lines changed

‎amd/comgr/test/unbundle_hip_test.c

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -182,11 +182,10 @@ int main(int Argc, char *Argv[]) {
182182
Status = amd_comgr_release_data(DataElement);
183183
checkError(Status, "amd_comgr_release_data");
184184

185-
// TODO: Re-enable after finalizing LLVM PR #122629
186-
//if (BytesSize != 0) {
187-
// printf("Bitcode host element size: %ld (expected 0)\n", BytesSize);
188-
// exit(1);
189-
//}
185+
if (!BytesSize) {
186+
printf("Bitcode host empty (expected non-empty)\n");
187+
exit(1);
188+
}
190189

191190
// bitcode hip-gfx900 element (non-empty)
192191
Status = amd_comgr_action_data_get_data(
@@ -248,11 +247,10 @@ int main(int Argc, char *Argv[]) {
248247
Status = amd_comgr_release_data(DataElement);
249248
checkError(Status, "amd_comgr_release_data");
250249

251-
// TODO: Re-enable after finalizing LLVM PR #122629
252-
//if (BytesSize != 0) {
253-
// printf("Object host element size: %ld (expected empty)\n", BytesSize);
254-
// exit(1);
255-
//}
250+
if (BytesSize != 0) {
251+
printf("Object host element size: %ld (expected empty)\n", BytesSize);
252+
exit(1);
253+
}
256254

257255
// object hip-gfx900 element (non-empty)
258256
Status = amd_comgr_action_data_get_data(
@@ -314,8 +312,8 @@ int main(int Argc, char *Argv[]) {
314312
Status = amd_comgr_release_data(DataElement);
315313
checkError(Status, "amd_comgr_release_data");
316314

317-
if (BytesSize != 8) {
318-
printf("Arvhive host element size: %ld (expected 8)\n", BytesSize);
315+
if (!BytesSize) {
316+
printf("Arvhive host empty (expected non-empty)\n");
319317
exit(1);
320318
}
321319

‎clang/docs/ClangOffloadBundler.rst

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -266,15 +266,14 @@ without differentiation based on offload kind.
266266
The target triple of the code object. See `Target Triple
267267
<https://clang.llvm.org/docs/CrossCompilation.html#target-triple>`_.
268268

269-
The bundler accepts target triples with or without the optional environment
270-
field:
269+
LLVM target triples can be with or without the optional environment field:
271270

272271
``<arch><sub>-<vendor>-<sys>``, or
273272
``<arch><sub>-<vendor>-<sys>-<env>``
274273

275-
However, in order to standardize outputs for tools that consume bitcode
276-
bundles, bundles written by the bundler internally use only the 4-field
277-
target triple:
274+
However, in order to standardize outputs for tools that consume bitcode bundles
275+
and to parse target ID containing dashes, the bundler only accepts target
276+
triples in the 4-field format:
278277

279278
``<arch><sub>-<vendor>-<sys>-<env>``
280279

@@ -543,4 +542,4 @@ The compressed offload bundle begins with a header followed by the compressed bi
543542
- **Compressed Data**:
544543
The actual compressed binary data follows the header. Its size can be inferred from the total size of the file minus the header size.
545544

546-
> **Note**: Version 3 of the format is under development. It uses 64-bit fields for Total File Size and Uncompressed Binary Size to support files larger than 4GB. To experiment with version 3, set the environment variable `COMPRESSED_BUNDLE_FORMAT_VERSION=3`. This support is experimental and not recommended for production use.
545+
> **Note**: Version 3 of the format is under development. It uses 64-bit fields for Total File Size and Uncompressed Binary Size to support files larger than 4GB. To experiment with version 3, set the environment variable `COMPRESSED_BUNDLE_FORMAT_VERSION=3`. This support is experimental and not recommended for production use.

‎clang/include/clang/Driver/OffloadBundler.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,11 @@ class CompressedOffloadBundle {
158158
static llvm::Expected<std::unique_ptr<llvm::MemoryBuffer>>
159159
decompress(const llvm::MemoryBuffer &Input, bool Verbose = false);
160160
};
161+
162+
/// Check whether the bundle id is in the following format:
163+
/// <kind>-<triple>[-<target id>[:target features]]
164+
/// <triple> := <arch>-<vendor>-<os>-<env>
165+
bool checkOffloadBundleID(const llvm::StringRef Str);
161166
} // namespace clang
162167

163168
#endif // LLVM_CLANG_DRIVER_OFFLOADBUNDLER_H

‎clang/lib/Driver/OffloadBundler.cpp

Lines changed: 52 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -83,32 +83,27 @@ OffloadTargetInfo::OffloadTargetInfo(const StringRef Target,
8383
const OffloadBundlerConfig &BC)
8484
: BundlerConfig(BC) {
8585

86-
// TODO: Add error checking from ClangOffloadBundler.cpp
87-
auto TargetFeatures = Target.split(':');
88-
auto TripleOrGPU = TargetFeatures.first.rsplit('-');
89-
90-
if (clang::StringToOffloadArch(TripleOrGPU.second) !=
91-
clang::OffloadArch::UNKNOWN) {
92-
auto KindTriple = TripleOrGPU.first.split('-');
93-
this->OffloadKind = KindTriple.first;
94-
95-
// Enforce optional env field to standardize bundles
96-
llvm::Triple t = llvm::Triple(KindTriple.second);
97-
this->Triple = llvm::Triple(t.getArchName(), t.getVendorName(),
98-
t.getOSName(), t.getEnvironmentName());
99-
100-
this->TargetID = Target.substr(Target.find(TripleOrGPU.second));
101-
} else {
102-
auto KindTriple = TargetFeatures.first.split('-');
103-
this->OffloadKind = KindTriple.first;
104-
105-
// Enforce optional env field to standardize bundles
106-
llvm::Triple t = llvm::Triple(KindTriple.second);
107-
this->Triple = llvm::Triple(t.getArchName(), t.getVendorName(),
108-
t.getOSName(), t.getEnvironmentName());
109-
86+
// <kind>-<triple>[-<target id>[:target features]]
87+
// <triple> := <arch>-<vendor>-<os>-<env>
88+
SmallVector<StringRef, 6> Components;
89+
Target.split(Components, '-', /*MaxSplit=*/5);
90+
assert((Components.size() == 5 || Components.size() == 6) &&
91+
"malformed target string");
92+
93+
StringRef TargetIdWithFeature =
94+
Components.size() == 6 ? Components.back() : "";
95+
StringRef TargetId = TargetIdWithFeature.split(':').first;
96+
if (!TargetId.empty() &&
97+
clang::StringToOffloadArch(TargetId) != clang::OffloadArch::UNKNOWN)
98+
this->TargetID = TargetIdWithFeature;
99+
else
110100
this->TargetID = "";
111-
}
101+
102+
this->OffloadKind = Components.front();
103+
ArrayRef<StringRef> TripleSlice{&Components[1], /*length=*/4};
104+
llvm::Triple T = llvm::Triple(llvm::join(TripleSlice, "-"));
105+
this->Triple = llvm::Triple(T.getArchName(), T.getVendorName(), T.getOSName(),
106+
T.getEnvironmentName());
112107
}
113108

114109
bool OffloadTargetInfo::hasHostKind() const {
@@ -148,7 +143,18 @@ bool OffloadTargetInfo::operator==(const OffloadTargetInfo &Target) const {
148143
}
149144

150145
std::string OffloadTargetInfo::str() const {
151-
return Twine(OffloadKind + "-" + Triple.str() + "-" + TargetID).str();
146+
std::string NormalizedTriple;
147+
// Unfortunately we need some special sauce for AMDGPU because all the runtime
148+
// assumes the triple to be "amdgcn-amd-amdhsa-" (empty environment) instead
149+
// of "amdgcn-amd-amdhsa-unknown". It's gonna be very tricky to patch
150+
// different layers of runtime.
151+
if (Triple.isAMDGPU()) {
152+
NormalizedTriple = Triple.normalize(Triple::CanonicalForm::THREE_IDENT);
153+
NormalizedTriple.push_back('-');
154+
} else {
155+
NormalizedTriple = Triple.normalize(Triple::CanonicalForm::FOUR_IDENT);
156+
}
157+
return Twine(OffloadKind + "-" + NormalizedTriple + "-" + TargetID).str();
152158
}
153159

154160
static StringRef getDeviceFileExtension(StringRef Device,
@@ -1507,6 +1513,9 @@ Error OffloadBundler::UnbundleFiles() {
15071513
StringMap<StringRef> Worklist;
15081514
auto Output = BundlerConfig.OutputFileNames.begin();
15091515
for (auto &Triple : BundlerConfig.TargetNames) {
1516+
if (!checkOffloadBundleID(Triple))
1517+
return createStringError(errc::invalid_argument,
1518+
"invalid bundle id from bundle config");
15101519
Worklist[Triple] = *Output;
15111520
++Output;
15121521
}
@@ -1526,6 +1535,9 @@ Error OffloadBundler::UnbundleFiles() {
15261535

15271536
StringRef CurTriple = **CurTripleOrErr;
15281537
assert(!CurTriple.empty());
1538+
if (!checkOffloadBundleID(CurTriple))
1539+
return createStringError(errc::invalid_argument,
1540+
"invalid bundle id read from the bundle");
15291541

15301542
auto Output = Worklist.begin();
15311543
for (auto E = Worklist.end(); Output != E; Output++) {
@@ -1584,6 +1596,8 @@ Error OffloadBundler::UnbundleFiles() {
15841596
return createFileError(E.second, EC);
15851597

15861598
// If this entry has a host kind, copy the input file to the output file.
1599+
// We don't need to check E.getKey() here through checkOffloadBundleID
1600+
// because the entire WorkList has been checked above.
15871601
auto OffloadInfo = OffloadTargetInfo(E.getKey(), BundlerConfig);
15881602
if (OffloadInfo.hasHostKind())
15891603
OutputFile.write(Input.getBufferStart(), Input.getBufferSize());
@@ -1813,6 +1827,10 @@ Error OffloadBundler::UnbundleArchive() {
18131827
// archive.
18141828
while (!CodeObject.empty()) {
18151829
SmallVector<StringRef> CompatibleTargets;
1830+
if (!checkOffloadBundleID(CodeObject)) {
1831+
return createStringError(errc::invalid_argument,
1832+
"Invalid bundle id read from code object");
1833+
}
18161834
auto CodeObjectInfo = OffloadTargetInfo(CodeObject, BundlerConfig);
18171835
if (getCompatibleOffloadTargets(CodeObjectInfo, CompatibleTargets,
18181836
BundlerConfig)) {
@@ -1894,3 +1912,11 @@ Error OffloadBundler::UnbundleArchive() {
18941912

18951913
return Error::success();
18961914
}
1915+
1916+
bool clang::checkOffloadBundleID(const llvm::StringRef Str) {
1917+
// <kind>-<triple>[-<target id>[:target features]]
1918+
// <triple> := <arch>-<vendor>-<os>-<env>
1919+
SmallVector<StringRef, 6> Components;
1920+
Str.split(Components, '-', /*MaxSplit=*/5);
1921+
return Components.size() == 5 || Components.size() == 6;
1922+
}

‎clang/lib/Driver/ToolChains/Clang.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9211,7 +9211,8 @@ void OffloadBundler::ConstructJob(Compilation &C, const JobAction &JA,
92119211
}
92129212
Triples += Action::GetOffloadKindName(CurKind);
92139213
Triples += '-';
9214-
Triples += CurTC->getTriple().normalize();
9214+
Triples +=
9215+
CurTC->getTriple().normalize(llvm::Triple::CanonicalForm::FOUR_IDENT);
92159216
if ((CurKind == Action::OFK_HIP || CurKind == Action::OFK_Cuda) &&
92169217
!StringRef(CurDep->getOffloadingArch()).empty()) {
92179218
Triples += '-';
@@ -9314,7 +9315,8 @@ void OffloadBundler::ConstructJobMultipleOutputs(
93149315
auto OffloadKind = Dep.DependentOffloadKind;
93159316
Triples += Action::GetOffloadKindName(OffloadKind);
93169317
Triples += '-';
9317-
Triples += Dep.DependentToolChain->getTriple().normalize();
9318+
Triples += Dep.DependentToolChain->getTriple().normalize(
9319+
llvm::Triple::CanonicalForm::FOUR_IDENT);
93189320
if ((Dep.DependentOffloadKind == Action::OFK_HIP ||
93199321
Dep.DependentOffloadKind == Action::OFK_Cuda) &&
93209322
!Dep.DependentBoundArch.empty()) {

‎clang/lib/Driver/ToolChains/CommonArgs.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2754,7 +2754,8 @@ static void GetSDLFromOffloadArchive(
27542754
SmallString<128> DeviceTriple;
27552755
DeviceTriple += Action::GetOffloadKindName(JA.getOffloadingDeviceKind());
27562756
DeviceTriple += '-';
2757-
std::string NormalizedTriple = T.getToolChain().getTriple().normalize();
2757+
std::string NormalizedTriple = T.getToolChain().getTriple().normalize(
2758+
llvm::Triple::CanonicalForm::FOUR_IDENT);
27582759
DeviceTriple += NormalizedTriple;
27592760
if (!Target.empty()) {
27602761
DeviceTriple += '-';

‎clang/lib/Driver/ToolChains/HIPUtility.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ static std::string normalizeForBundler(const llvm::Triple &T,
4545
return HasTargetID ? (T.getArchName() + "-" + T.getVendorName() + "-" +
4646
T.getOSName() + "-" + T.getEnvironmentName())
4747
.str()
48-
: T.normalize();
48+
: T.normalize(llvm::Triple::CanonicalForm::FOUR_IDENT);
4949
}
5050

5151
// Collect undefined __hip_fatbin* and __hip_gpubin_handle* symbols from all

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,18 @@
1515
// Check code object compatibility for archive unbundling
1616
//
1717
// Create few code object bundles and archive them to create an input archive
18-
// 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
18+
// 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
1919
// 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
2020
// 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
2121
// 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
2222
// RUN: llvm-ar cr %t.input-archive.a %t.simple.bundle %t.targetID1.bundle %t.targetID2.bundle %t.targetID3.bundle
2323

2424
// Tests to check compatibility between Bundle Entry ID formats i.e. between presence/absence of extra hyphen in case of missing environment field
25-
// 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
25+
// 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
2626
// BUNDLECOMPATIBILITY: Compatible: Exact match: [CodeObject: openmp-amdgcn-amd-amdhsa--gfx906] : [Target: openmp-amdgcn-amd-amdhsa--gfx906]
2727
// BUNDLECOMPATIBILITY: Compatible: Exact match: [CodeObject: openmp-amdgcn-amd-amdhsa--gfx908] : [Target: openmp-amdgcn-amd-amdhsa--gfx908]
2828

29-
// 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
29+
// 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
3030
// HIPOpenMPCOMPATIBILITY: Compatible: Target IDs are compatible [CodeObject: openmp-amdgcn-amd-amdhsa--gfx906] : [Target: hip-amdgcn-amd-amdhsa--gfx906]
3131
// HIPOpenMPCOMPATIBILITY: Compatible: Target IDs are compatible [CodeObject: openmp-amdgcn-amd-amdhsa--gfx908] : [Target: hipv4-amdgcn-amd-amdhsa--gfx908]
3232

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

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,20 +15,12 @@
1515
//
1616
// Check code object compatibility for archive unbundling
1717
//
18-
// Create an object bundle with and without env fields
19-
// 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
20-
// 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
18+
// Create an object bundle
19+
// 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
2120

22-
23-
// Unbundle bundle.no.env while providing targets with env
24-
// 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
25-
// BUNDLE-NO-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa--gfx906] : [Target: hip-amdgcn-amd-amdhsa--gfx906]
26-
// BUNDLE-NO-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa--gfx908] : [Target: hip-amdgcn-amd-amdhsa--gfx908]
27-
28-
// Unbundle bundle.env while providing targets with no env
29-
// 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
30-
// BUNDLE-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa--gfx906] : [Target: hip-amdgcn-amd-amdhsa--gfx906]
31-
// BUNDLE-ENV: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa--gfx908] : [Target: hip-amdgcn-amd-amdhsa--gfx908]
21+
// RUN: clang-offload-bundler -unbundle -type=o -targets=hip-amdgcn-amd-amdhsa--gfx906,hip-amdgcn-amd-amdhsa--gfx908 -input=%t.bundle -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
22+
// BUNDLE: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa--gfx906] : [Target: hip-amdgcn-amd-amdhsa--gfx906]
23+
// BUNDLE: Compatible: Exact match: [CodeObject: hip-amdgcn-amd-amdhsa--gfx908] : [Target: hip-amdgcn-amd-amdhsa--gfx908]
3224

3325
// Some code so that we can create a binary out of this file.
3426
int A = 0;

0 commit comments

Comments
 (0)