llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-backend-amdgpu Author: Matt Arsenault (arsenm) <details> <summary>Changes</summary> We had grown 2 parallel parsing implementations for triple+gpu name+feature flag target ID strings. Mostly eliminate the redundant clang version. Co-authored-by: Claude (Opus 4.8) --- Patch is 36.59 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/209845.diff 12 Files Affected: - (modified) clang/include/clang/Basic/TargetID.h (+10-34) - (modified) clang/lib/Basic/TargetID.cpp (+28-165) - (modified) clang/lib/Basic/Targets/AMDGPU.cpp (+17-14) - (modified) clang/lib/Basic/Targets/AMDGPU.h (+18-14) - (modified) clang/lib/Driver/Driver.cpp (+18-9) - (modified) clang/lib/Driver/OffloadBundler.cpp (+39-43) - (modified) clang/lib/Driver/ToolChains/AMDGPU.cpp (+48-50) - (modified) clang/lib/Driver/ToolChains/AMDGPU.h (+10-13) - (modified) clang/tools/clang-offload-bundler/ClangOffloadBundler.cpp (+14-9) - (modified) llvm/include/llvm/TargetParser/AMDGPUTargetParser.h (+1-1) - (modified) llvm/lib/TargetParser/AMDGPUTargetParser.cpp (+1-1) - (modified) llvm/unittests/TargetParser/TargetParserTest.cpp (+3-3) ``````````diff diff --git a/clang/include/clang/Basic/TargetID.h b/clang/include/clang/Basic/TargetID.h index 902151d76556d..8871b76859fd7 100644 --- a/clang/include/clang/Basic/TargetID.h +++ b/clang/include/clang/Basic/TargetID.h @@ -9,53 +9,29 @@ #ifndef LLVM_CLANG_BASIC_TARGETID_H #define LLVM_CLANG_BASIC_TARGETID_H -#include "llvm/ADT/SmallVector.h" -#include "llvm/ADT/StringMap.h" +#include "llvm/ADT/ArrayRef.h" #include "llvm/TargetParser/Triple.h" #include <optional> -#include <set> +#include <string> +#include <utility> namespace clang { -/// Get all feature strings that can be used in target ID for \p Processor. -/// Target ID is a processor name with optional feature strings -/// postfixed by a plus or minus sign delimited by colons, e.g. -/// gfx908:xnack+:sramecc-. Each processor have a limited -/// number of predefined features when showing up in a target ID. -llvm::SmallVector<llvm::StringRef, 4> -getAllPossibleTargetIDFeatures(const llvm::Triple &T, - llvm::StringRef Processor); - /// Get processor name from target ID. /// Returns canonical processor name or empty if the processor name is invalid. llvm::StringRef getProcessorFromTargetID(const llvm::Triple &T, llvm::StringRef OffloadArch); -/// Parse a target ID to get processor and feature map. -/// Returns canonicalized processor name or std::nullopt if the target ID is -/// invalid. Returns target ID features in \p FeatureMap if it is not null -/// pointer. This function assumes \p OffloadArch is a valid target ID. -/// If the target ID contains feature+, map it to true. -/// If the target ID contains feature-, map it to false. -/// If the target ID does not contain a feature (default), do not map it. -std::optional<llvm::StringRef> parseTargetID(const llvm::Triple &T, - llvm::StringRef OffloadArch, - llvm::StringMap<bool> *FeatureMap); - -/// Returns canonical target ID, assuming \p Processor is canonical and all -/// entries in \p Features are valid. -std::string getCanonicalTargetID(llvm::StringRef Processor, - const llvm::StringMap<bool> &Features); +/// A device triple paired with a target ID (processor and feature modifiers) +/// for that triple, e.g. {amdgcn-amd-amdhsa, "gfx906:xnack+"}. +using TargetIDEntry = std::pair<const llvm::Triple &, llvm::StringRef>; /// Get the conflicted pair of target IDs for a compilation or a bundled code -/// object, assuming \p TargetIDs are canonicalized. If there is no conflicts, -/// returns std::nullopt. +/// object. Two entries conflict when they resolve to the same processor but +/// disagree on whether a feature (xnack/sramecc) is explicitly specified. If +/// there is no conflict, returns std::nullopt. std::optional<std::pair<llvm::StringRef, llvm::StringRef>> -getConflictTargetIDCombination(const std::set<llvm::StringRef> &TargetIDs); - -/// Check whether the provided target ID is compatible with the requested -/// target ID. -bool isCompatibleTargetID(llvm::StringRef Provided, llvm::StringRef Requested); +getConflictTargetIDCombination(llvm::ArrayRef<TargetIDEntry> Entries); /// Sanitize a target ID string for use in a file name. /// Replaces invalid characters (like ':') with safe characters (like '@'). diff --git a/clang/lib/Basic/TargetID.cpp b/clang/lib/Basic/TargetID.cpp index 67f429607ef27..a179d26645c5b 100644 --- a/clang/lib/Basic/TargetID.cpp +++ b/clang/lib/Basic/TargetID.cpp @@ -7,186 +7,49 @@ //===----------------------------------------------------------------------===// #include "clang/Basic/TargetID.h" -#include "llvm/ADT/STLExtras.h" -#include "llvm/ADT/SmallSet.h" -#include "llvm/ADT/SmallVector.h" #include "llvm/Support/Path.h" #include "llvm/TargetParser/AMDGPUTargetParser.h" -#include "llvm/TargetParser/Triple.h" -#include <map> -#include <optional> -#include <string> namespace clang { -static llvm::SmallVector<llvm::StringRef, 4> -getAllPossibleAMDGPUTargetIDFeatures(const llvm::Triple &T, - llvm::StringRef Proc) { - // Entries in returned vector should be in alphabetical order. - llvm::SmallVector<llvm::StringRef, 4> Ret; - auto ProcKind = T.isAMDGCN() ? llvm::AMDGPU::parseArchAMDGCN(Proc) - : llvm::AMDGPU::parseArchR600(Proc); - if (ProcKind == llvm::AMDGPU::GK_NONE) - return Ret; - auto Features = T.isAMDGCN() ? llvm::AMDGPU::getArchAttrAMDGCN(ProcKind) - : llvm::AMDGPU::getArchAttrR600(ProcKind); - if (Features & llvm::AMDGPU::FEATURE_SRAMECC) - Ret.push_back("sramecc"); - // Only allow xnack in target ID if the processor supports on/off modes. - if (Features & llvm::AMDGPU::FEATURE_XNACK_ON_OFF_MODES) - Ret.push_back("xnack"); - return Ret; -} - -llvm::SmallVector<llvm::StringRef, 4> -getAllPossibleTargetIDFeatures(const llvm::Triple &T, - llvm::StringRef Processor) { - llvm::SmallVector<llvm::StringRef, 4> Ret; - if (T.isAMDGPU()) - return getAllPossibleAMDGPUTargetIDFeatures(T, Processor); - return Ret; -} - -/// Returns canonical processor name or empty string if \p Processor is invalid. -static llvm::StringRef getCanonicalProcessorName(const llvm::Triple &T, - llvm::StringRef Processor) { - if (T.isAMDGPU()) - return llvm::AMDGPU::getCanonicalArchName(T, Processor); - return Processor; -} - llvm::StringRef getProcessorFromTargetID(const llvm::Triple &T, - llvm::StringRef TargetID) { - auto Split = TargetID.split(':'); - return getCanonicalProcessorName(T, Split.first); -} - -// Parse a target ID with format checking only. Do not check whether processor -// name or features are valid for the processor. -// -// A target ID is a processor name followed by a list of target features -// delimited by colon. Each target feature is a string post-fixed by a plus -// or minus sign, e.g. gfx908:sramecc+:xnack-. -static std::optional<llvm::StringRef> -parseTargetIDWithFormatCheckingOnly(llvm::StringRef TargetID, - llvm::StringMap<bool> *FeatureMap) { - llvm::StringRef Processor; - - if (TargetID.empty()) - return llvm::StringRef(); - - auto Split = TargetID.split(':'); - Processor = Split.first; - if (Processor.empty()) - return std::nullopt; - - auto Features = Split.second; - if (Features.empty()) - return Processor; - - llvm::StringMap<bool> LocalFeatureMap; - if (!FeatureMap) - FeatureMap = &LocalFeatureMap; - - while (!Features.empty()) { - auto Splits = Features.split(':'); - if (Splits.first.empty()) - return std::nullopt; - auto Sign = Splits.first.back(); - auto Feature = Splits.first.drop_back(); - if (Sign != '+' && Sign != '-') - return std::nullopt; - bool IsOn = Sign == '+'; - // Each feature can only show up at most once in target ID. - if (!FeatureMap->try_emplace(Feature, IsOn).second) - return std::nullopt; - Features = Splits.second; - } - return Processor; -} - -std::optional<llvm::StringRef> -parseTargetID(const llvm::Triple &T, llvm::StringRef TargetID, - llvm::StringMap<bool> *FeatureMap) { - auto OptionalProcessor = - parseTargetIDWithFormatCheckingOnly(TargetID, FeatureMap); - - if (!OptionalProcessor) - return std::nullopt; - - llvm::StringRef Processor = getCanonicalProcessorName(T, *OptionalProcessor); - if (Processor.empty()) - return std::nullopt; - - llvm::SmallSet<llvm::StringRef, 4> AllFeatures( - llvm::from_range, getAllPossibleTargetIDFeatures(T, Processor)); - - for (auto &&F : *FeatureMap) - if (!AllFeatures.count(F.first())) - return std::nullopt; - - return Processor; -} - -// A canonical target ID is a target ID containing a canonical processor name -// and features in alphabetical order. -std::string getCanonicalTargetID(llvm::StringRef Processor, - const llvm::StringMap<bool> &Features) { - std::string TargetID = Processor.str(); - std::map<const llvm::StringRef, bool> OrderedMap; - for (const auto &F : Features) - OrderedMap[F.first()] = F.second; - for (const auto &F : OrderedMap) - TargetID = TargetID + ':' + F.first.str() + (F.second ? "+" : "-"); - return TargetID; + llvm::StringRef OffloadArch) { + auto Split = OffloadArch.split(':'); + if (T.isAMDGPU()) + return llvm::AMDGPU::getCanonicalArchName(T, Split.first); + return Split.first; } // For a specific processor, a feature either shows up in all target IDs, or -// does not show up in any target IDs. Otherwise the target ID combination -// is invalid. +// does not show up in any target IDs. Otherwise the target ID combination is +// invalid. std::optional<std::pair<llvm::StringRef, llvm::StringRef>> -getConflictTargetIDCombination(const std::set<llvm::StringRef> &TargetIDs) { +getConflictTargetIDCombination(llvm::ArrayRef<TargetIDEntry> Entries) { struct Info { llvm::StringRef TargetID; - llvm::StringMap<bool> Features; - Info(llvm::StringRef TargetID, const llvm::StringMap<bool> &Features) - : TargetID(TargetID), Features(Features) {} + bool HasXnack; + bool HasSramEcc; }; - llvm::StringMap<Info> FeatureMap; - for (auto &&ID : TargetIDs) { - llvm::StringMap<bool> Features; - llvm::StringRef Proc = *parseTargetIDWithFormatCheckingOnly(ID, &Features); - auto [Loc, Inserted] = FeatureMap.try_emplace(Proc, ID, Features); - if (!Inserted) { - auto &ExistingFeatures = Loc->second.Features; - if (llvm::any_of(Features, [&](auto &F) { - return ExistingFeatures.count(F.first()) == 0; - })) - return std::make_pair(Loc->second.TargetID, ID); - } - } - return std::nullopt; -} -bool isCompatibleTargetID(llvm::StringRef Provided, llvm::StringRef Requested) { - llvm::StringMap<bool> ProvidedFeatures, RequestedFeatures; - llvm::StringRef ProvidedProc = - *parseTargetIDWithFormatCheckingOnly(Provided, &ProvidedFeatures); - llvm::StringRef RequestedProc = - *parseTargetIDWithFormatCheckingOnly(Requested, &RequestedFeatures); - if (ProvidedProc != RequestedProc) - return false; - for (const auto &F : ProvidedFeatures) { - auto Loc = RequestedFeatures.find(F.first()); - // The default (unspecified) value of a feature is 'All', which can match - // either 'On' or 'Off'. - if (Loc == RequestedFeatures.end()) - return false; - // If a feature is specified, it must have exact match. - if (Loc->second != F.second) - return false; + llvm::SmallDenseMap<llvm::AMDGPU::GPUKind, Info> Seen; + for (const auto &[T, ID] : Entries) { + std::optional<llvm::AMDGPU::TargetID> Parsed = + llvm::AMDGPU::TargetID::parse(T, ID); + if (!Parsed) + continue; + + // A feature is present in a target ID only when an explicit '+'/'-' + // modifier is given, not when it is left unspecified. + Info Cur{ID, Parsed->isXnackOnOrOff(), Parsed->isSramEccOnOrOff()}; + auto [Loc, Inserted] = Seen.try_emplace(Parsed->getGPUKind(), Cur); + if (Inserted) + continue; + + const Info &Prev = Loc->second; + if (Cur.HasXnack != Prev.HasXnack || Cur.HasSramEcc != Prev.HasSramEcc) + return std::make_pair(Prev.TargetID, ID); } - return true; + return std::nullopt; } std::string sanitizeTargetIDInFileName(llvm::StringRef TargetID) { diff --git a/clang/lib/Basic/Targets/AMDGPU.cpp b/clang/lib/Basic/Targets/AMDGPU.cpp index 3fd9643373383..49bc99a34a5d5 100644 --- a/clang/lib/Basic/Targets/AMDGPU.cpp +++ b/clang/lib/Basic/Targets/AMDGPU.cpp @@ -295,22 +295,25 @@ void AMDGPUTargetInfo::getTargetDefines(const LangOptions &Opts, Twine("__")); Builder.defineMacro("__amdgcn_processor__", Twine("\"") + Twine(CanonName) + Twine("\"")); - Builder.defineMacro( - "__amdgcn_target_id__", - Twine("\"") + - Twine(getCanonicalTargetID(getArchNameAMDGCN(GPUKind), - OffloadArchFeatures)) + - Twine("\"")); - for (auto F : getAllPossibleTargetIDFeatures(getTriple(), CanonName)) { - auto Loc = OffloadArchFeatures.find(F); - if (Loc != OffloadArchFeatures.end()) { - std::string NewF = F.str(); + llvm::AMDGPU::TargetID TargetID(GPUKind, getTriple(), XnackSetting, + SramEccSetting); + Builder.defineMacro("__amdgcn_target_id__", + Twine("\"") + + Twine(TargetID.getCanonicalTargetIDString()) + + Twine("\"")); + auto DefineFeatureMacro = [&](StringRef Feature, + llvm::AMDGPU::TargetIDSetting Setting) { + if (Setting == llvm::AMDGPU::TargetIDSetting::On || + Setting == llvm::AMDGPU::TargetIDSetting::Off) { + std::string NewF = Feature.str(); llvm::replace(NewF, '-', '_'); - Builder.defineMacro(Twine("__amdgcn_feature_") + Twine(NewF) + - Twine("__"), - Loc->second ? "1" : "0"); + Builder.defineMacro( + Twine("__amdgcn_feature_") + Twine(NewF) + Twine("__"), + Setting == llvm::AMDGPU::TargetIDSetting::On ? "1" : "0"); } - } + }; + DefineFeatureMacro("xnack", XnackSetting); + DefineFeatureMacro("sramecc", SramEccSetting); } if (Opts.AtomicIgnoreDenormalMode) diff --git a/clang/lib/Basic/Targets/AMDGPU.h b/clang/lib/Basic/Targets/AMDGPU.h index 89ba561ef302d..b2117542c2edd 100644 --- a/clang/lib/Basic/Targets/AMDGPU.h +++ b/clang/lib/Basic/Targets/AMDGPU.h @@ -42,13 +42,13 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo { /// Whether having image instructions. bool HasImage = false; - /// Target ID is device name followed by optional feature name postfixed - /// by plus or minus sign delimitted by colon, e.g. gfx908:xnack+:sramecc-. - /// If the target ID contains feature+, map it to true. - /// If the target ID contains feature-, map it to false. - /// If the target ID does not contain a feature (default), do not map it. - llvm::StringMap<bool> OffloadArchFeatures; - std::string TargetID; + /// Explicit xnack/sramecc target-id feature settings from the command line, + /// e.g. gfx908:xnack+:sramecc-. "Unsupported" means the feature was not + /// specified (or is not a valid target-id modifier for the processor). + llvm::AMDGPU::TargetIDSetting XnackSetting = + llvm::AMDGPU::TargetIDSetting::Unsupported; + llvm::AMDGPU::TargetIDSetting SramEccSetting = + llvm::AMDGPU::TargetIDSetting::Unsupported; bool hasFP64() const { return getTriple().isAMDGCN() || @@ -462,8 +462,7 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo { bool handleTargetFeatures(std::vector<std::string> &Features, DiagnosticsEngine &Diags) override { HasFullBFloat16 = true; - auto TargetIDFeatures = - getAllPossibleTargetIDFeatures(getTriple(), getArchNameAMDGCN(GPUKind)); + unsigned ArchAttr = llvm::AMDGPU::getArchAttrAMDGCN(GPUKind); for (const auto &F : Features) { assert(F.front() == '+' || F.front() == '-'); if (F == "+wavefrontsize64") @@ -474,12 +473,17 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo { CUMode = false; else if (F == "+image-insts") HasImage = true; - bool IsOn = F.front() == '+'; + llvm::AMDGPU::TargetIDSetting Setting = + F.front() == '+' ? llvm::AMDGPU::TargetIDSetting::On + : llvm::AMDGPU::TargetIDSetting::Off; StringRef Name = StringRef(F).drop_front(); - if (!llvm::is_contained(TargetIDFeatures, Name)) - continue; - assert(!OffloadArchFeatures.contains(Name)); - OffloadArchFeatures[Name] = IsOn; + // xnack is a valid target-id modifier only when the processor supports + // on/off modes; sramecc when the processor supports sramecc. + if (Name == "xnack" && + (ArchAttr & llvm::AMDGPU::FEATURE_XNACK_ON_OFF_MODES)) + XnackSetting = Setting; + else if (Name == "sramecc" && (ArchAttr & llvm::AMDGPU::FEATURE_SRAMECC)) + SramEccSetting = Setting; } return true; } diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp index e606cdc4c1cf8..b0ebfc9e99c53 100644 --- a/clang/lib/Driver/Driver.cpp +++ b/clang/lib/Driver/Driver.cpp @@ -108,6 +108,7 @@ #include "llvm/Support/TarWriter.h" #include "llvm/Support/VirtualFileSystem.h" #include "llvm/Support/raw_ostream.h" +#include "llvm/TargetParser/AMDGPUTargetParser.h" #include "llvm/TargetParser/Host.h" #include "llvm/TargetParser/RISCVISAInfo.h" #include <cstdlib> // ::getenv @@ -4851,14 +4852,17 @@ static StringRef getCanonicalArchString(Compilation &C, if (IsNVIDIAOffloadArch(Arch)) return Args.MakeArgStringRef(OffloadArchToString(Arch)); - if (IsAMDOffloadArch(Arch)) { - llvm::StringMap<bool> Features; - std::optional<StringRef> Arch = parseTargetID(Triple, ArchStr, &Features); - if (!Arch) { + // AMDGCN target IDs carry a processor and xnack/sramecc modifiers to + // canonicalize. Other AMD offload arches (e.g. the amdgcnspirv pseudo-arch on + // a SPIR-V triple) have no target-id features and pass through unchanged. + if (IsAMDOffloadArch(Arch) && Triple.isAMDGCN()) { + std::optional<llvm::AMDGPU::TargetID> ID = + llvm::AMDGPU::TargetID::parse(Triple, ArchStr); + if (!ID) { C.getDriver().Diag(clang::diag::err_drv_bad_target_id) << ArchStr; return StringRef(); } - return Args.MakeArgStringRef(getCanonicalTargetID(*Arch, Features)); + return Args.MakeArgStringRef(ID->getCanonicalTargetIDString()); } // If the input isn't CUDA or HIP just return the architecture. @@ -4869,13 +4873,18 @@ static StringRef getCanonicalArchString(Compilation &C, /// incompatible pair if a conflict occurs. static std::optional<std::pair<llvm::StringRef, llvm::StringRef>> getConflictOffloadArchCombination(const llvm::DenseSet<StringRef> &Archs, - llvm::Triple Triple) { + const llvm::Triple &Triple) { if (!Triple.isAMDGPU()) return std::nullopt; - std::set<StringRef> ArchSet; - llvm::copy(Archs, std::inserter(ArchSet, ArchSet.begin())); - return getConflictTargetIDCombination(ArchSet); + // Sort for a deterministic conflicting pair in the diagnostic. + llvm::SmallVector<StringRef> ArchList(Archs.begin(), Archs.end()); + llvm::sort(ArchList); + + llvm::SmallVector<clang::TargetIDEntry> Entries; + for (StringRef Arch : ArchList) + Entries.emplace_back(Triple, Arch); + return getConflictTargetIDCombination(Entries); } llvm::SmallVector<BoundArch> diff --git a/clang/lib/Driver/OffloadBundler.cpp b/clang/lib/Driver/OffloadBundler.cpp index 8e4d44071ef55..41c09a6dec5ed 100644 --- a/clang/lib/Driver/OffloadBundler.cpp +++ b/clang/lib/Driver/OffloadBundler.cpp @@ -48,6 +48,7 @@ #include "llvm/Support/Timer.h" #include "llvm/Support/WithColor.h" #include "llvm/Support/raw_ostream.h" +#include "llvm/TargetParser/AMDGPUTargetParser.h" #include "llvm/TargetParser/Host.h" #include "llvm/TargetParser/Triple.h" #include <algorithm> @@ -1115,15 +1116,15 @@ bool isCodeObjectCompatible(const OffloadTargetInfo &CodeObjectInfo, } // Incompatible if Processors mismatch. - llvm::StringMap<bool> CodeObjectFeatureMap, TargetFeatureMap; - std::optional<StringRef> CodeObjectProc = clang::parseTargetID( - CodeObjectInfo.Triple, CodeObjectInfo.TargetID, &CodeObjectFeatureMap); - std::optional<StringRef> TargetProc = clang::parseTargetID( - TargetInfo.Triple, TargetInfo.TargetID, &TargetFeatureMap); - - // Both TargetProc and CodeObjectProc can't be empty here. - if (!TargetProc || !CodeObjectProc || - CodeObjectProc.value() != TargetProc.value()) { + std::optional<llvm::AMDGPU::TargetID> CodeObjectID = + llvm::AMDGPU::TargetID::parse(CodeObjectInfo.Triple, + CodeObjectInfo.TargetID); + std::optional<llvm::AMDGPU::TargetID> TargetID = + llvm::A... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/209845 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
