llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-llvm-binary-utilities Author: Matt Arsenault (arsenm) <details> <summary>Changes</summary> Reverts llvm/llvm-project#<!-- -->209135 This shouldn't be doing logical linking compatibility --- Full diff: https://github.com/llvm/llvm-project/pull/209407.diff 6 Files Affected: - (modified) clang/test/OffloadTools/clang-linker-wrapper/linker-wrapper.c (-57) - (modified) clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp (+2-1) - (modified) llvm/include/llvm/TargetParser/AMDGPUTargetParser.h (-8) - (modified) llvm/lib/Object/OffloadBinary.cpp (+22-17) - (modified) llvm/lib/TargetParser/AMDGPUTargetParser.cpp (+35-49) - (modified) llvm/unittests/TargetParser/TargetParserTest.cpp (-48) ``````````diff diff --git a/clang/test/OffloadTools/clang-linker-wrapper/linker-wrapper.c b/clang/test/OffloadTools/clang-linker-wrapper/linker-wrapper.c index f4bac21181a9e..0e018dc3af8c9 100644 --- a/clang/test/OffloadTools/clang-linker-wrapper/linker-wrapper.c +++ b/clang/test/OffloadTools/clang-linker-wrapper/linker-wrapper.c @@ -217,63 +217,6 @@ __attribute__((visibility("protected"), used)) int x; // ARCH-ALL: clang{{.*}} -o {{.*}}.img -dumpdir a.out.amdgcn.gfx90a.img. --target=amdgcn-amd-amdhsa -mcpu=gfx90a -Wl,--no-undefined {{.*}}.o {{.*}}.o // ARCH-ALL: clang{{.*}} -o {{.*}}.img -dumpdir a.out.amdgcn.gfx908.img. --target=amdgcn-amd-amdhsa -mcpu=gfx908 -Wl,--no-undefined {{.*}}.o {{.*}}.o -// Two images with the same ISA but different triple spellings (the legacy -// "amdgcn" alias and the canonical "amdgpu") must merge into a single device -// link, not produce two separate device images. -// RUN: llvm-offload-binary -o %t-legacy.out \ -// RUN: --image=file=%t.elf.o,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx90a -// RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t-legacy.o -fembed-offload-object=%t-legacy.out -// RUN: llvm-offload-binary -o %t-canon.out \ -// RUN: --image=file=%t.elf.o,kind=openmp,triple=amdgpu-amd-amdhsa,arch=gfx90a -// RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t-canon.o -fembed-offload-object=%t-canon.out -// RUN: clang-linker-wrapper --host-triple=x86_64-unknown-linux-gnu --dry-run \ -// RUN: --linker-path=/usr/bin/ld %t-legacy.o %t-canon.o -o a.out 2>&1 | FileCheck %s --check-prefix=AMDGPU-TRIPLE-MERGE - -// AMDGPU-TRIPLE-MERGE: clang{{.*}} --target=amdgcn-amd-amdhsa -mcpu=gfx90a -Wl,--no-undefined {{.*}}.o {{.*}}.o -// AMDGPU-TRIPLE-MERGE-NOT: clang{{.*}} --target=amdgcn-amd-amdhsa -mcpu=gfx90a -Wl,--no-undefined - -// Two images whose triples encode the ISA in the subarch (amdgpu9.0a-amd-amdhsa) -// and carry no separate arch must merge into a single device link. -// RUN: llvm-offload-binary -o %t-sub1.out \ -// RUN: --image=file=%t.elf.o,kind=openmp,triple=amdgpu9.0a-amd-amdhsa -// RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t-sub1.o -fembed-offload-object=%t-sub1.out -// RUN: llvm-offload-binary -o %t-sub2.out \ -// RUN: --image=file=%t.elf.o,kind=openmp,triple=amdgpu9.0a-amd-amdhsa -// RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t-sub2.o -fembed-offload-object=%t-sub2.out -// RUN: clang-linker-wrapper --host-triple=x86_64-unknown-linux-gnu --dry-run \ -// RUN: --linker-path=/usr/bin/ld %t-sub1.o %t-sub2.o -o a.out 2>&1 | FileCheck %s --check-prefix=AMDGPU-SUBARCH-MERGE - -// AMDGPU-SUBARCH-MERGE: clang{{.*}} --target=amdgpu9.0a-amd-amdhsa -Wl,--no-undefined {{.*}}.o {{.*}}.o -// AMDGPU-SUBARCH-MERGE-NOT: clang{{.*}} --target=amdgpu9.0a-amd-amdhsa -Wl,--no-undefined - -// A major-family subarch triple (amdgpu9-amd-amdhsa) acts as a wildcard that -// merges into a specific member of that family (amdgpu9.00-amd-amdhsa). -// RUN: llvm-offload-binary -o %t-specific.out \ -// RUN: --image=file=%t.elf.o,kind=openmp,triple=amdgpu9.00-amd-amdhsa -// RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t-specific.o -fembed-offload-object=%t-specific.out -// RUN: llvm-offload-binary -o %t-major.out \ -// RUN: --image=file=%t.elf.o,kind=openmp,triple=amdgpu9-amd-amdhsa -// RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t-major.o -fembed-offload-object=%t-major.out -// RUN: clang-linker-wrapper --host-triple=x86_64-unknown-linux-gnu --dry-run \ -// RUN: --linker-path=/usr/bin/ld %t-specific.o %t-major.o -o a.out 2>&1 | FileCheck %s --check-prefix=AMDGPU-MAJOR-SUBARCH-MERGE - -// AMDGPU-MAJOR-SUBARCH-MERGE: clang{{.*}} --target=amdgpu9.00-amd-amdhsa -Wl,--no-undefined {{.*}}.o {{.*}}.o -// AMDGPU-MAJOR-SUBARCH-MERGE-NOT: clang{{.*}} --target={{.*}}-amd-amdhsa -Wl,--no-undefined - -// Two distinct GPUs in the same major family are not interchangeable and must -// not merge. -// RUN: llvm-offload-binary -o %t-gfx900.out \ -// RUN: --image=file=%t.elf.o,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx900 -// RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t-gfx900.o -fembed-offload-object=%t-gfx900.out -// RUN: llvm-offload-binary -o %t-gfx906.out \ -// RUN: --image=file=%t.elf.o,kind=openmp,triple=amdgcn-amd-amdhsa,arch=gfx906 -// RUN: %clang -cc1 %s -triple x86_64-unknown-linux-gnu -emit-obj -o %t-gfx906.o -fembed-offload-object=%t-gfx906.out -// RUN: clang-linker-wrapper --host-triple=x86_64-unknown-linux-gnu --dry-run \ -// RUN: --linker-path=/usr/bin/ld %t-gfx900.o %t-gfx906.o -o a.out 2>&1 | FileCheck %s --check-prefix=AMDGPU-SAME-MAJOR-NO-MERGE - -// AMDGPU-SAME-MAJOR-NO-MERGE-DAG: clang{{.*}} -mcpu=gfx900 -Wl,--no-undefined {{.*}}.o -// AMDGPU-SAME-MAJOR-NO-MERGE-DAG: clang{{.*}} -mcpu=gfx906 -Wl,--no-undefined {{.*}}.o - // RUN: llvm-offload-binary -o %t.out \ // RUN: --image=file=%t.elf.o,kind=openmp,triple=x86_64-unknown-linux-gnu \ // RUN: --image=file=%t.elf.o,kind=openmp,triple=x86_64-unknown-linux-gnu diff --git a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp index afa84eea41aae..12b6cae3ea792 100644 --- a/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp +++ b/clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp @@ -1414,7 +1414,8 @@ getDeviceInput(const ArgList &Args) { OffloadFile::TargetID Target = Binary; SmallVector<OffloadFile::TargetID> CompatibleTargets; for (const auto &[ID, Input] : InputFiles) - if (Target == ID || object::areTargetsCompatible(Target, ID)) + if (Target.first == ID.first && + clang::isCompatibleTargetID(Target.second, ID.second)) CompatibleTargets.emplace_back(ID); // Seed a new image when no existing target can provide for this input. diff --git a/llvm/include/llvm/TargetParser/AMDGPUTargetParser.h b/llvm/include/llvm/TargetParser/AMDGPUTargetParser.h index d5e2609d20f25..ab8cb05ed80f6 100644 --- a/llvm/include/llvm/TargetParser/AMDGPUTargetParser.h +++ b/llvm/include/llvm/TargetParser/AMDGPUTargetParser.h @@ -167,10 +167,6 @@ class LLVM_ABI TargetID { TargetID(GPUKind Arch, const Triple &TT, TargetIDSetting XnackSetting, TargetIDSetting SramEccSetting); - /// Construct a TargetID from a triple \p TT and the processor+features string - /// e.g. "gfx90a", "gfx90a:xnack+:sramecc-", "". - TargetID(const Triple &TT, StringRef TargetIDStr); - ~TargetID() = default; /// \return True if the current xnack setting is not "Unsupported". @@ -239,10 +235,6 @@ class LLVM_ABI TargetID { static std::optional<TargetID> parseTargetIDString(StringRef TargetIDDirective); - /// Returns true if a device image built for *this can satisfy a request for - /// \p Other (i.e. they are compatible and can be grouped together). - bool isCompatibleWith(const TargetID &Other) const; - void print(raw_ostream &OS) const; std::string toString() const; diff --git a/llvm/lib/Object/OffloadBinary.cpp b/llvm/lib/Object/OffloadBinary.cpp index d1d01f9f7a667..e5c9f4dae131d 100644 --- a/llvm/lib/Object/OffloadBinary.cpp +++ b/llvm/lib/Object/OffloadBinary.cpp @@ -22,7 +22,6 @@ #include "llvm/Object/ObjectFile.h" #include "llvm/Support/Alignment.h" #include "llvm/Support/SourceMgr.h" -#include "llvm/TargetParser/AMDGPUTargetParser.h" using namespace llvm; using namespace llvm::object; @@ -454,25 +453,31 @@ bool object::areTargetsCompatible(const OffloadFile::TargetID &LHS, if (LHS == RHS) return false; - llvm::Triple LHSTT(LHS.first); - llvm::Triple RHSTT(RHS.first); - - // The AMDGPU target requires target-id aware checks (base processor plus - // xnack/sramecc features), which also handle the generic wildcard and the - // legacy "amdgcn"/"amdgpu" triple spellings. - if (LHSTT.isAMDGPU()) { - AMDGPU::TargetID LTID(LHSTT, LHS.second); - AMDGPU::TargetID RTID(RHSTT, RHS.second); - return LTID.isCompatibleWith(RTID); - } - - // For other targets the triples must be compatible. - if (!LHSTT.isCompatibleWith(RHSTT)) + // The triples must match at all times. + if (LHS.first != RHS.first) return false; - // If the architecture is "generic" we assume it is always compatible. + // If the architecture is "all" we assume it is always compatible. if (LHS.second == "generic" || RHS.second == "generic") return true; - return LHS.second == RHS.second; + // Only The AMDGPU target requires additional checks. + llvm::Triple T(LHS.first); + if (!T.isAMDGPU()) + return false; + + // The base processor must always match. + if (LHS.second.split(":").first != RHS.second.split(":").first) + return false; + + // Check combintions of on / off features that must match. + if (LHS.second.contains("xnack+") && RHS.second.contains("xnack-")) + return false; + if (LHS.second.contains("xnack-") && RHS.second.contains("xnack+")) + return false; + if (LHS.second.contains("sramecc-") && RHS.second.contains("sramecc+")) + return false; + if (LHS.second.contains("sramecc+") && RHS.second.contains("sramecc-")) + return false; + return true; } diff --git a/llvm/lib/TargetParser/AMDGPUTargetParser.cpp b/llvm/lib/TargetParser/AMDGPUTargetParser.cpp index c5f4247117d1b..bf169c5b5ef42 100644 --- a/llvm/lib/TargetParser/AMDGPUTargetParser.cpp +++ b/llvm/lib/TargetParser/AMDGPUTargetParser.cpp @@ -11,6 +11,7 @@ //===----------------------------------------------------------------------===// #include "llvm/TargetParser/AMDGPUTargetParser.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringSwitch.h" #include "llvm/ADT/Twine.h" @@ -916,28 +917,6 @@ getTargetIDSettingFromFeatureString(StringRef FeatureString) { llvm_unreachable("Malformed feature string"); } -// Derive the architecture from the processor name in \p TargetIDStr. "generic" -// and the empty processor name act as a wildcard. -static GPUKind getGPUKindFromTargetID(const Triple &TT, StringRef TargetIDStr) { - StringRef CPUName = TargetIDStr.split(':').first; - return (CPUName.empty() || CPUName == "generic") - ? getGPUKindFromSubArch(TT.getSubArch()) - : parseArchAMDGCN(CPUName); -} - -TargetID::TargetID(const Triple &TT, StringRef TargetIDStr) - : TargetID(getGPUKindFromTargetID(TT, TargetIDStr), TT, - TargetIDSetting::Unsupported, TargetIDSetting::Unsupported) { - // Default xnack/sramecc to the "Any" wildcard when the architecture supports - // them, then apply any explicit feature overrides from the target-id string. - unsigned ArchAttr = getArchAttrAMDGCN(Arch); - if (ArchAttr & FEATURE_XNACK) - XnackSetting = TargetIDSetting::Any; - if (ArchAttr & FEATURE_SRAMECC) - SramEccSetting = TargetIDSetting::Any; - setTargetIDFromTargetIDStream(TargetIDStr); -} - void TargetID::setTargetIDFromTargetIDStream(StringRef TargetID) { SmallVector<StringRef, 3> TargetIDSplit; TargetID.split(TargetIDSplit, ':'); @@ -963,10 +942,40 @@ TargetID::parseTargetIDString(StringRef TargetIDDirective) { if (!TT.isAMDGCN()) return std::nullopt; - // The processor+features field must be present, even if empty (the ISA can - // be encoded in the triple's subarch, e.g. - // "amdgpu12.50-amd-amdhsa-unknown-"). - return TargetID(TT, Parts[4]); + SmallVector<StringRef, 3> FeatureSplit; + Parts[4].split(FeatureSplit, ':'); + if (FeatureSplit.empty()) + return std::nullopt; + + StringRef CPUName = FeatureSplit[0]; + + // Prefer the explicitly named processor so the parsed target id reflects it + // (e.g. for validation against the triple subarch). The processor field may + // be empty when the ISA is already encoded in the triple's subarch + // (e.g. "amdgpu12.50-amd-amdhsa-unknown-"), in which case derive the arch + // from the subarch. + GPUKind Arch = CPUName.empty() ? getGPUKindFromSubArch(TT.getSubArch()) + : parseArchAMDGCN(CPUName); + + unsigned ArchAttr = getArchAttrAMDGCN(Arch); + + // Determine xnack/sramecc support based on the architecture attributes. + TargetIDSetting XnackSetting = (ArchAttr & FEATURE_XNACK) + ? TargetIDSetting::Any + : TargetIDSetting::Unsupported; + TargetIDSetting SramEccSetting = (ArchAttr & FEATURE_SRAMECC) + ? TargetIDSetting::Any + : TargetIDSetting::Unsupported; + + for (StringRef FeatureString : + ArrayRef<StringRef>(FeatureSplit).drop_front(1)) { + if (FeatureString.starts_with("xnack")) + XnackSetting = getTargetIDSettingFromFeatureString(FeatureString); + else if (FeatureString.starts_with("sramecc")) + SramEccSetting = getTargetIDSettingFromFeatureString(FeatureString); + } + + return TargetID(Arch, TT, XnackSetting, SramEccSetting); } void TargetID::print(raw_ostream &StreamRep) const { @@ -999,26 +1008,3 @@ bool TargetID::operator==(const TargetID &Other) const { SramEccSetting == Other.SramEccSetting && IsAMDHSA == Other.IsAMDHSA && TargetTripleString == Other.TargetTripleString; } - -static bool areFeatureSettingsCompatible(TargetIDSetting A, TargetIDSetting B) { - return A == TargetIDSetting::Any || B == TargetIDSetting::Any || A == B; -} - -bool TargetID::isCompatibleWith(const TargetID &Other) const { - // The triples must be compatible. - if (!Triple(getTargetTripleString()) - .isCompatibleWith(Triple(Other.getTargetTripleString()))) - return false; - - // The processors must be compatible. A generic/major-family image (its - // subarch is the major-family subarch, or the GPU is unknown) acts as a - // wildcard that merges into a specific image group. - Triple::SubArchType SubA = getSubArch(Arch); - Triple::SubArchType SubB = getSubArch(Other.Arch); - if (!isSubArchCompatible(SubA, SubB)) - return false; - - // The xnack/sramecc settings must not conflict. - return areFeatureSettingsCompatible(XnackSetting, Other.XnackSetting) && - areFeatureSettingsCompatible(SramEccSetting, Other.SramEccSetting); -} diff --git a/llvm/unittests/TargetParser/TargetParserTest.cpp b/llvm/unittests/TargetParser/TargetParserTest.cpp index 1b6216f9ae68f..231e3cb306298 100644 --- a/llvm/unittests/TargetParser/TargetParserTest.cpp +++ b/llvm/unittests/TargetParser/TargetParserTest.cpp @@ -2828,52 +2828,4 @@ TEST(TargetParserTest, testAMDGPUgetIsaVersionFromSubArch) { (AMDGPU::IsaVersion{0, 0, 0})); } -TEST(TargetParserTest, testAMDGPUTargetIDCompat) { - using AMDGPU::TargetID; - Triple AMDHSA("amdgcn-amd-amdhsa"); - Triple AMDHSACanon("amdgpu-amd-amdhsa"); - - auto Compatible = [](const TargetID &A, const TargetID &B) { - // Compatibility is symmetric for the cases exercised here. - EXPECT_EQ(A.isCompatibleWith(B), B.isCompatibleWith(A)); - return A.isCompatibleWith(B); - }; - - // Exact match is compatible. - EXPECT_TRUE( - Compatible(TargetID(AMDHSA, "gfx90a"), TargetID(AMDHSA, "gfx90a"))); - - // Different processors are not compatible. - EXPECT_FALSE( - Compatible(TargetID(AMDHSA, "gfx90a"), TargetID(AMDHSA, "gfx908"))); - - // "generic" and empty act as a wildcard that merges into a specific image, - // in both directions. - EXPECT_TRUE( - Compatible(TargetID(AMDHSA, "generic"), TargetID(AMDHSA, "gfx90a"))); - EXPECT_TRUE(Compatible(TargetID(AMDHSA, ""), TargetID(AMDHSA, "gfx908"))); - - // Major-subarch wildcard triple merges into a specific processor. - Triple AMDHSAMajor9("amdgpu9-amd-amdhsa"); - EXPECT_TRUE( - Compatible(TargetID(AMDHSAMajor9, ""), TargetID(AMDHSA, "gfx900"))); - - // Explicit On vs Off features must not match. - EXPECT_FALSE(Compatible(TargetID(AMDHSA, "gfx90a:xnack+"), - TargetID(AMDHSA, "gfx90a:xnack-"))); - EXPECT_FALSE(Compatible(TargetID(AMDHSA, "gfx90a:sramecc+"), - TargetID(AMDHSA, "gfx90a:sramecc-"))); - - // "Any"/"Unsupported" settings act as wildcards: a plain gfx90a (xnack "Any") - // merges with an explicit xnack+ or xnack- request. - EXPECT_TRUE(Compatible(TargetID(AMDHSA, "gfx90a"), - TargetID(AMDHSA, "gfx90a:xnack+"))); - EXPECT_TRUE(Compatible(TargetID(AMDHSA, "gfx90a"), - TargetID(AMDHSA, "gfx90a:xnack-"))); - - // The legacy "amdgcn" and canonical "amdgpu" triple spellings merge. - EXPECT_TRUE( - Compatible(TargetID(AMDHSA, "gfx90a"), TargetID(AMDHSACanon, "gfx90a"))); -} - } // namespace `````````` </details> https://github.com/llvm/llvm-project/pull/209407 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
