From ef9637b5e74358d7e1a3f3a57624d3d184306900 Mon Sep 17 00:00:00 2001 From: Min-Yih Hsu Date: Fri, 14 Nov 2025 14:22:07 -0800 Subject: [PATCH 1/6] [RISCV] Introduce a new syntax for specifying tuning feature strings Co-Authored-By: Sam Elliott --- .../llvm/TargetParser/RISCVTargetParser.h | 4 + llvm/lib/Target/RISCV/RISCVFeatures.td | 48 ++++----- llvm/lib/TargetParser/RISCVTargetParser.cpp | 99 +++++++++++++++++++ .../TargetParser/RISCVTargetParserTest.cpp | 88 +++++++++++++++++ .../TableGen/Basic/RISCVTargetDefEmitter.cpp | 49 ++++++++- 5 files changed, 265 insertions(+), 23 deletions(-) diff --git a/llvm/include/llvm/TargetParser/RISCVTargetParser.h b/llvm/include/llvm/TargetParser/RISCVTargetParser.h index 2ac58a539d5ee..2c6a59a59a993 100644 --- a/llvm/include/llvm/TargetParser/RISCVTargetParser.h +++ b/llvm/include/llvm/TargetParser/RISCVTargetParser.h @@ -16,6 +16,7 @@ #include "llvm/ADT/StringRef.h" #include "llvm/Support/Compiler.h" +#include "llvm/Support/Error.h" #include "llvm/Support/MathExtras.h" #include "llvm/Support/raw_ostream.h" @@ -54,6 +55,9 @@ static constexpr unsigned RVVBytesPerBlock = RVVBitsPerBlock / 8; LLVM_ABI void getFeaturesForCPU(StringRef CPU, SmallVectorImpl &EnabledFeatures, bool NeedPlus = false); +LLVM_ABI void getAllTuneFeatures(SmallVectorImpl &TuneFeatures); +LLVM_ABI Error parseTuneFeatureString( + StringRef TFString, SmallVectorImpl &TuneFeatures); LLVM_ABI bool parseCPU(StringRef CPU, bool IsRV64); LLVM_ABI bool parseTuneCPU(StringRef CPU, bool IsRV64); LLVM_ABI StringRef getMArchFromMcpu(StringRef CPU); diff --git a/llvm/lib/Target/RISCV/RISCVFeatures.td b/llvm/lib/Target/RISCV/RISCVFeatures.td index 0b964c4808d8a..ef5203445b55c 100644 --- a/llvm/lib/Target/RISCV/RISCVFeatures.td +++ b/llvm/lib/Target/RISCV/RISCVFeatures.td @@ -1740,6 +1740,10 @@ def HasVendorXSMTVDot // LLVM specific features and extensions //===----------------------------------------------------------------------===// +class RISCVTuneFeature implied = []> + : SubtargetFeature; + // Feature32Bit exists to mark CPUs that support RV32 to distinguish them from // tuning CPU names. def Feature32Bit @@ -1788,46 +1792,46 @@ def FeatureUnalignedVectorMem "loads and stores">; def TuneNLogNVRGather - : SubtargetFeature<"log-vrgather", "RISCVVRGatherCostModel", "NLog2N", + : RISCVTuneFeature<"log-vrgather", "RISCVVRGatherCostModel", "NLog2N", "Has vrgather.vv with LMUL*log2(LMUL) latency">; -def TunePostRAScheduler : SubtargetFeature<"use-postra-scheduler", +def TunePostRAScheduler : RISCVTuneFeature<"use-postra-scheduler", "UsePostRAScheduler", "true", "Schedule again after register allocation">; -def TuneDisableMISchedLoadClustering : SubtargetFeature<"disable-misched-load-clustering", +def TuneDisableMISchedLoadClustering : RISCVTuneFeature<"disable-misched-load-clustering", "EnableMISchedLoadClustering", "false", "Disable load clustering in the machine scheduler">; -def TuneDisableMISchedStoreClustering : SubtargetFeature<"disable-misched-store-clustering", +def TuneDisableMISchedStoreClustering : RISCVTuneFeature<"disable-misched-store-clustering", "EnableMISchedStoreClustering", "false", "Disable store clustering in the machine scheduler">; -def TuneDisablePostMISchedLoadClustering : SubtargetFeature<"disable-postmisched-load-clustering", +def TuneDisablePostMISchedLoadClustering : RISCVTuneFeature<"disable-postmisched-load-clustering", "EnablePostMISchedLoadClustering", "false", "Disable PostRA load clustering in the machine scheduler">; -def TuneDisablePostMISchedStoreClustering : SubtargetFeature<"disable-postmisched-store-clustering", +def TuneDisablePostMISchedStoreClustering : RISCVTuneFeature<"disable-postmisched-store-clustering", "EnablePostMISchedStoreClustering", "false", "Disable PostRA store clustering in the machine scheduler">; def TuneDisableLatencySchedHeuristic - : SubtargetFeature<"disable-latency-sched-heuristic", "DisableLatencySchedHeuristic", "true", + : RISCVTuneFeature<"disable-latency-sched-heuristic", "DisableLatencySchedHeuristic", "true", "Disable latency scheduling heuristic">; def TunePredictableSelectIsExpensive - : SubtargetFeature<"predictable-select-expensive", "PredictableSelectIsExpensive", "true", + : RISCVTuneFeature<"predictable-select-expensive", "PredictableSelectIsExpensive", "true", "Prefer likely predicted branches over selects">; def TuneOptimizedZeroStrideLoad - : SubtargetFeature<"optimized-zero-stride-load", "HasOptimizedZeroStrideLoad", + : RISCVTuneFeature<"optimized-zero-stride-load", "HasOptimizedZeroStrideLoad", "true", "Optimized (perform fewer memory operations)" "zero-stride vector load">; foreach nf = {2-8} in def TuneOptimizedNF#nf#SegmentLoadStore : - SubtargetFeature<"optimized-nf"#nf#"-segment-load-store", + RISCVTuneFeature<"optimized-nf"#nf#"-segment-load-store", "HasOptimizedNF"#nf#"SegmentLoadStore", "true", "vlseg"#nf#"eN.v and vsseg"#nf#"eN.v are " "implemented as a wide memory op and shuffle">; def TuneVLDependentLatency - : SubtargetFeature<"vl-dependent-latency", "HasVLDependentLatency", "true", + : RISCVTuneFeature<"vl-dependent-latency", "HasVLDependentLatency", "true", "Latency of vector instructions is dependent on the " "dynamic value of vl">; @@ -1839,50 +1843,50 @@ def Experimental // and instead split over multiple cycles. DLEN refers to the datapath width // that can be done in parallel. def TuneDLenFactor2 - : SubtargetFeature<"dlen-factor-2", "DLenFactor2", "true", + : RISCVTuneFeature<"dlen-factor-2", "DLenFactor2", "true", "Vector unit DLEN(data path width) is half of VLEN">; def TuneNoDefaultUnroll - : SubtargetFeature<"no-default-unroll", "EnableDefaultUnroll", "false", + : RISCVTuneFeature<"no-default-unroll", "EnableDefaultUnroll", "false", "Disable default unroll preference.">; // SiFive 7 is able to fuse integer ALU operations with a preceding branch // instruction. def TuneShortForwardBranchOpt - : SubtargetFeature<"short-forward-branch-opt", "HasShortForwardBranchOpt", + : RISCVTuneFeature<"short-forward-branch-opt", "HasShortForwardBranchOpt", "true", "Enable short forward branch optimization">; def HasShortForwardBranchOpt : Predicate<"Subtarget->hasShortForwardBranchOpt()">; def NoShortForwardBranchOpt : Predicate<"!Subtarget->hasShortForwardBranchOpt()">; def TuneShortForwardBranchIMinMax - : SubtargetFeature<"short-forward-branch-i-minmax", "HasShortForwardBranchIMinMax", + : RISCVTuneFeature<"short-forward-branch-i-minmax", "HasShortForwardBranchIMinMax", "true", "Enable short forward branch optimization for min,max instructions in Zbb", [TuneShortForwardBranchOpt]>; def TuneShortForwardBranchIMul - : SubtargetFeature<"short-forward-branch-i-mul", "HasShortForwardBranchIMul", + : RISCVTuneFeature<"short-forward-branch-i-mul", "HasShortForwardBranchIMul", "true", "Enable short forward branch optimization for mul instruction", [TuneShortForwardBranchOpt]>; // Some subtargets require a S2V transfer buffer to move scalars into vectors. // FIXME: Forming .vx/.vf/.wx/.wf can reduce register pressure. def TuneNoSinkSplatOperands - : SubtargetFeature<"no-sink-splat-operands", "SinkSplatOperands", + : RISCVTuneFeature<"no-sink-splat-operands", "SinkSplatOperands", "false", "Disable sink splat operands to enable .vx, .vf," ".wx, and .wf instructions">; def TunePreferWInst - : SubtargetFeature<"prefer-w-inst", "PreferWInst", "true", + : RISCVTuneFeature<"prefer-w-inst", "PreferWInst", "true", "Prefer instructions with W suffix">; def TuneConditionalCompressedMoveFusion - : SubtargetFeature<"conditional-cmv-fusion", "HasConditionalCompressedMoveFusion", + : RISCVTuneFeature<"conditional-cmv-fusion", "HasConditionalCompressedMoveFusion", "true", "Enable branch+c.mv fusion">; def HasConditionalMoveFusion : Predicate<"Subtarget->hasConditionalMoveFusion()">; def NoConditionalMoveFusion : Predicate<"!Subtarget->hasConditionalMoveFusion()">; def TuneHasSingleElementVecFP64 - : SubtargetFeature<"single-element-vec-fp64", "HasSingleElementVectorFP64", "true", + : RISCVTuneFeature<"single-element-vec-fp64", "HasSingleElementVectorFP64", "true", "Certain vector FP64 operations produce a single result " "element per cycle">; @@ -1899,11 +1903,11 @@ def TuneVentanaVeyron : SubtargetFeature<"ventana-veyron", "RISCVProcFamily", "V def TuneAndes45 : SubtargetFeature<"andes45", "RISCVProcFamily", "Andes45", "Andes 45-Series processors">; -def TuneVXRMPipelineFlush : SubtargetFeature<"vxrm-pipeline-flush", "HasVXRMPipelineFlush", +def TuneVXRMPipelineFlush : RISCVTuneFeature<"vxrm-pipeline-flush", "HasVXRMPipelineFlush", "true", "VXRM writes causes pipeline flush">; def TunePreferVsetvliOverReadVLENB - : SubtargetFeature<"prefer-vsetvli-over-read-vlenb", + : RISCVTuneFeature<"prefer-vsetvli-over-read-vlenb", "PreferVsetvliOverReadVLENB", "true", "Prefer vsetvli over read vlenb CSR to calculate VLEN">; diff --git a/llvm/lib/TargetParser/RISCVTargetParser.cpp b/llvm/lib/TargetParser/RISCVTargetParser.cpp index 5ea63a973ea37..aa2039a00a550 100644 --- a/llvm/lib/TargetParser/RISCVTargetParser.cpp +++ b/llvm/lib/TargetParser/RISCVTargetParser.cpp @@ -12,7 +12,11 @@ //===----------------------------------------------------------------------===// #include "llvm/TargetParser/RISCVTargetParser.h" +#include "llvm/ADT/SetOperations.h" +#include "llvm/ADT/SmallSet.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringExtras.h" +#include "llvm/ADT/StringSet.h" #include "llvm/ADT/StringSwitch.h" #include "llvm/TargetParser/RISCVISAInfo.h" @@ -145,6 +149,101 @@ void getFeaturesForCPU(StringRef CPU, EnabledFeatures.push_back(F.substr(1)); } +using RISCVImpliedTuneFeature = std::pair; + +#define GET_TUNE_FEATURES +#define GET_IMPLIED_TUNE_FEATURES +#include "llvm/TargetParser/RISCVTargetParserDef.inc" + +void getAllTuneFeatures(SmallVectorImpl &Features) { + Features.assign(std::begin(TuneFeatures), std::end(TuneFeatures)); +} + +Error parseTuneFeatureString(StringRef TFString, + SmallVectorImpl &ResFeatures) { + const StringSet<> AllTuneFeatureSet(llvm::from_range, TuneFeatures); + using SmallStringSet = SmallSet; + + TFString = TFString.trim(); + // Note: StringSet is not really ergnomic to use in this case here. + SmallStringSet PositiveFeatures; + SmallStringSet NegativeFeatures; + // Phase 1: Collect explicit features. + StringRef FeatureStr; + do { + std::tie(FeatureStr, TFString) = TFString.split(","); + if (AllTuneFeatureSet.count(FeatureStr)) { + if (!PositiveFeatures.insert(FeatureStr).second) + return createStringError(inconvertibleErrorCode(), + "cannot specify more than one instance of '" + + Twine(FeatureStr) + "'"); + } else if (FeatureStr.starts_with("no-")) { + // Check if this is a negative feature, like `no-foo` for `foo`. + StringRef ActualFeature = FeatureStr.drop_front(3); + if (AllTuneFeatureSet.count(ActualFeature)) { + if (!NegativeFeatures.insert(ActualFeature).second) + return createStringError( + inconvertibleErrorCode(), + "cannot specify more than one instance of '" + Twine(FeatureStr) + + "'"); + } + } else { + return createStringError(inconvertibleErrorCode(), + "unrecognized tune feature directive '" + + Twine(FeatureStr) + "'"); + } + } while (!TFString.empty()); + + auto Intersection = + llvm::set_intersection(PositiveFeatures, NegativeFeatures); + if (!Intersection.empty()) { + std::string IntersectedStr = join(Intersection, "', '"); + return createStringError(inconvertibleErrorCode(), + "Feature(s) '" + Twine(IntersectedStr) + + "' cannot appear in both " + "positive and negative directives"); + } + + // Phase 2: Derive implied features. + StringMap> ImpliedFeatureMap; + StringMap> InverseImpliedFeatureMap; + for (auto [Feature, ImpliedFeature] : ImpliedTuneFeatures) { + ImpliedFeatureMap[Feature].push_back(ImpliedFeature); + InverseImpliedFeatureMap[ImpliedFeature].push_back(Feature); + } + + for (StringRef PF : PositiveFeatures) { + auto ItFeatures = ImpliedFeatureMap.find(PF); + if (ItFeatures != ImpliedFeatureMap.end()) + PositiveFeatures.insert(ItFeatures->second.begin(), + ItFeatures->second.end()); + } + for (StringRef NF : NegativeFeatures) { + auto ItFeatures = InverseImpliedFeatureMap.find(NF); + if (ItFeatures != InverseImpliedFeatureMap.end()) + NegativeFeatures.insert(ItFeatures->second.begin(), + ItFeatures->second.end()); + } + + Intersection = llvm::set_intersection(PositiveFeatures, NegativeFeatures); + if (!Intersection.empty()) { + std::string IntersectedStr = join(Intersection, "', '"); + return createStringError(inconvertibleErrorCode(), + "Feature(s) '" + Twine(IntersectedStr) + + "' were implied by both " + "positive and negative directives"); + } + + // Export the result. + const std::string PosPrefix("+"); + const std::string NegPrefix("-"); + for (StringRef PF : PositiveFeatures) + ResFeatures.emplace_back(PosPrefix + PF.str()); + for (StringRef NF : NegativeFeatures) + ResFeatures.emplace_back(NegPrefix + NF.str()); + + return Error::success(); +} } // namespace RISCV namespace RISCVVType { diff --git a/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp b/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp index 63ac8f993ecdc..e85b08a5df5ff 100644 --- a/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp +++ b/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp @@ -7,6 +7,8 @@ //===----------------------------------------------------------------------===// #include "llvm/TargetParser/RISCVTargetParser.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" #include "gtest/gtest.h" using namespace llvm; @@ -30,4 +32,90 @@ TEST(RISCVVType, CheckSameRatioLMUL) { EXPECT_EQ(RISCVVType::LMUL_F2, RISCVVType::getSameRatioLMUL(8, RISCVVType::LMUL_F4, 16)); } + +TEST(RISCVTuneFeature, AllTuneFeatures) { + SmallVector AllTuneFeatures; + RISCV::getAllTuneFeatures(AllTuneFeatures); + EXPECT_EQ(AllTuneFeatures.size(), 28U); + for (auto F : {"conditional-cmv-fusion", + "dlen-factor-2", + "disable-latency-sched-heuristic", + "disable-misched-load-clustering", + "disable-misched-store-clustering", + "disable-postmisched-load-clustering", + "disable-postmisched-store-clustering", + "single-element-vec-fp64", + "log-vrgather", + "no-default-unroll", + "no-sink-splat-operands", + "optimized-nf2-segment-load-store", + "optimized-nf3-segment-load-store", + "optimized-nf4-segment-load-store", + "optimized-nf5-segment-load-store", + "optimized-nf6-segment-load-store", + "optimized-nf7-segment-load-store", + "optimized-nf8-segment-load-store", + "optimized-zero-stride-load", + "use-postra-scheduler", + "predictable-select-expensive", + "prefer-vsetvli-over-read-vlenb", + "prefer-w-inst", + "short-forward-branch-i-minmax", + "short-forward-branch-i-mul", + "short-forward-branch-opt", + "vl-dependent-latency", + "vxrm-pipeline-flush"}) + EXPECT_TRUE(is_contained(AllTuneFeatures, F)); +} + +TEST(RISCVTuneFeature, LegalTuneFeatureStrings) { + SmallVector Result; + EXPECT_FALSE(errorToBool(RISCV::parseTuneFeatureString( + "log-vrgather,no-short-forward-branch-opt,vl-dependent-latency", + Result))); + EXPECT_TRUE(is_contained(Result, "+log-vrgather")); + EXPECT_TRUE(is_contained(Result, "+vl-dependent-latency")); + EXPECT_TRUE(is_contained(Result, "-short-forward-branch-opt")); + EXPECT_TRUE(is_contained(Result, "-short-forward-branch-i-minmax")); + EXPECT_TRUE(is_contained(Result, "-short-forward-branch-i-mul")); + + Result.clear(); + EXPECT_FALSE(errorToBool(RISCV::parseTuneFeatureString( + "no-short-forward-branch-i-mul,short-forward-branch-i-minmax", Result))); + EXPECT_TRUE(is_contained(Result, "+short-forward-branch-i-minmax")); + EXPECT_TRUE(is_contained(Result, "+short-forward-branch-opt")); + EXPECT_TRUE(is_contained(Result, "-short-forward-branch-i-mul")); + + Result.clear(); + EXPECT_FALSE(errorToBool(RISCV::parseTuneFeatureString( + "no-no-default-unroll,no-sink-splat-operands", Result))); + EXPECT_TRUE(is_contained(Result, "+no-sink-splat-operands")); + EXPECT_TRUE(is_contained(Result, "-no-default-unroll")); +} + +TEST(RISCVTuneFeature, UnrecognizedTuneFeature) { + SmallVector Result; + EXPECT_EQ(toString(RISCV::parseTuneFeatureString("32bit", Result)), + "unrecognized tune feature directive '32bit'"); +} + +TEST(RISCVTuneFeature, DuplicatedFeatures) { + SmallVector Result; + EXPECT_EQ(toString(RISCV::parseTuneFeatureString("log-vrgather,log-vrgather", + Result)), + "cannot specify more than one instance of 'log-vrgather'"); + + EXPECT_EQ(toString(RISCV::parseTuneFeatureString( + "log-vrgather,no-log-vrgather,short-forward-branch-i-mul,no-" + "short-forward-branch-i-mul", + Result)), + "Feature(s) 'log-vrgather', 'short-forward-branch-i-mul' cannot " + "appear in both positive and negative directives"); + + EXPECT_EQ( + toString(RISCV::parseTuneFeatureString( + "short-forward-branch-i-mul,no-short-forward-branch-opt", Result)), + "Feature(s) 'short-forward-branch-i-mul', 'short-forward-branch-opt' " + "were implied by both positive and negative directives"); +} } // namespace diff --git a/llvm/utils/TableGen/Basic/RISCVTargetDefEmitter.cpp b/llvm/utils/TableGen/Basic/RISCVTargetDefEmitter.cpp index f7959376adc4a..7420fa439ecd4 100644 --- a/llvm/utils/TableGen/Basic/RISCVTargetDefEmitter.cpp +++ b/llvm/utils/TableGen/Basic/RISCVTargetDefEmitter.cpp @@ -14,6 +14,7 @@ #include "llvm/ADT/DenseSet.h" #include "llvm/Support/Format.h" #include "llvm/Support/RISCVISAUtils.h" +#include "llvm/TableGen/Error.h" #include "llvm/TableGen/Record.h" #include "llvm/TableGen/TableGenBackend.h" @@ -259,7 +260,52 @@ static void emitRISCVExtensionBitmask(const RecordKeeper &RK, raw_ostream &OS) { << "},\n"; } OS << "};\n"; - OS << "#endif\n"; + OS << "#endif\n\n"; +} + +static void emitRISCVTuneFeatures(const RecordKeeper &RK, raw_ostream &OS) { + std::vector TuneFeatureRecords = + RK.getAllDerivedDefinitionsIfDefined("RISCVTuneFeature"); + + SmallVector TuneFeatures; + // A list of {Feature -> Implied Feature} + SmallVector> ImpliedFeatures; + for (const auto *R : TuneFeatureRecords) { + StringRef FeatureName = R->getValueAsString("Name"); + TuneFeatures.push_back(FeatureName); + std::vector Implies = R->getValueAsListOfDefs("Implies"); + for (const auto *ImpliedRecord : Implies) { + if (!ImpliedRecord->isSubClassOf("RISCVTuneFeature") || + ImpliedRecord == R) { + PrintError(ImpliedRecord, + "RISCVTuneFeature can only imply other RISCVTuneFeatures"); + PrintFatalNote(R, "implied by this RISCVTuneFeature"); + } + ImpliedFeatures.emplace_back(FeatureName, + ImpliedRecord->getValueAsString("Name")); + } + } + + OS << "#ifdef GET_TUNE_FEATURES\n"; + OS << "#undef GET_TUNE_FEATURES\n\n"; + + OS << "static const char *TuneFeatures[] = {\n"; + for (StringRef Feature : TuneFeatures) + OS.indent(4) << "\"" << Feature << "\",\n"; + OS << "};\n\n"; + + OS << "#endif // GET_TUNE_FEATURES\n\n"; + + OS << "#ifdef GET_IMPLIED_TUNE_FEATURES\n"; + OS << "#undef GET_IMPLIED_TUNE_FEATURES\n\n"; + + OS << "static const RISCVImpliedTuneFeature ImpliedTuneFeatures[] = {\n"; + for (auto [Feature, ImpliedFeature] : ImpliedFeatures) + OS.indent(4) << "{" << "\"" << Feature << "\", \"" << ImpliedFeature + << "\"},\n"; + OS << "};\n\n"; + + OS << "#endif // GET_IMPLIED_TUNE_FEATURES\n"; } static void emitRiscvTargetDef(const RecordKeeper &RK, raw_ostream &OS) { @@ -267,6 +313,7 @@ static void emitRiscvTargetDef(const RecordKeeper &RK, raw_ostream &OS) { emitRISCVProfiles(RK, OS); emitRISCVProcs(RK, OS); emitRISCVExtensionBitmask(RK, OS); + emitRISCVTuneFeatures(RK, OS); } static TableGen::Emitter::Opt X("gen-riscv-target-def", emitRiscvTargetDef, From 0af6194f69d01bbda51c1df5f2a4f2e99c80d7ad Mon Sep 17 00:00:00 2001 From: Min-Yih Hsu Date: Tue, 18 Nov 2025 14:06:48 -0800 Subject: [PATCH 2/6] fixup! Raise unrecognized (explicit) feature as warning --- .../llvm/TargetParser/RISCVTargetParser.h | 16 +++++++++ llvm/lib/TargetParser/RISCVTargetParser.cpp | 34 +++++++++---------- .../TargetParser/RISCVTargetParserTest.cpp | 5 ++- 3 files changed, 37 insertions(+), 18 deletions(-) diff --git a/llvm/include/llvm/TargetParser/RISCVTargetParser.h b/llvm/include/llvm/TargetParser/RISCVTargetParser.h index 2c6a59a59a993..5cb339ff58a75 100644 --- a/llvm/include/llvm/TargetParser/RISCVTargetParser.h +++ b/llvm/include/llvm/TargetParser/RISCVTargetParser.h @@ -48,6 +48,22 @@ struct CPUInfo { bool is64Bit() const { return DefaultMarch.starts_with("rv64"); } }; +/// Fatal errors encountered during parsing. +struct ParserError : public ErrorInfo { + using ErrorInfo::ErrorInfo; + explicit ParserError(const Twine &S) + : ErrorInfo(S, inconvertibleErrorCode()) {} + static char ID; +}; + +/// Warnings encountered during parsing. +struct ParserWarning : public ErrorInfo { + using ErrorInfo::ErrorInfo; + explicit ParserWarning(const Twine &S) + : ErrorInfo(S, inconvertibleErrorCode()) {} + static char ID; +}; + // We use 64 bits as the known part in the scalable vector types. static constexpr unsigned RVVBitsPerBlock = 64; static constexpr unsigned RVVBytesPerBlock = RVVBitsPerBlock / 8; diff --git a/llvm/lib/TargetParser/RISCVTargetParser.cpp b/llvm/lib/TargetParser/RISCVTargetParser.cpp index aa2039a00a550..4914ba798f56d 100644 --- a/llvm/lib/TargetParser/RISCVTargetParser.cpp +++ b/llvm/lib/TargetParser/RISCVTargetParser.cpp @@ -23,6 +23,9 @@ namespace llvm { namespace RISCV { +char ParserError::ID = 0; +char ParserWarning::ID = 0; + enum CPUKind : unsigned { #define PROC(ENUM, NAME, DEFAULT_MARCH, FAST_SCALAR_UNALIGN, \ FAST_VECTOR_UNALIGN, MVENDORID, MARCHID, MIMPID) \ @@ -174,23 +177,22 @@ Error parseTuneFeatureString(StringRef TFString, std::tie(FeatureStr, TFString) = TFString.split(","); if (AllTuneFeatureSet.count(FeatureStr)) { if (!PositiveFeatures.insert(FeatureStr).second) - return createStringError(inconvertibleErrorCode(), - "cannot specify more than one instance of '" + - Twine(FeatureStr) + "'"); + return make_error( + "cannot specify more than one instance of '" + Twine(FeatureStr) + + "'"); } else if (FeatureStr.starts_with("no-")) { // Check if this is a negative feature, like `no-foo` for `foo`. StringRef ActualFeature = FeatureStr.drop_front(3); if (AllTuneFeatureSet.count(ActualFeature)) { if (!NegativeFeatures.insert(ActualFeature).second) - return createStringError( - inconvertibleErrorCode(), + return make_error( "cannot specify more than one instance of '" + Twine(FeatureStr) + - "'"); + "'"); } } else { - return createStringError(inconvertibleErrorCode(), - "unrecognized tune feature directive '" + - Twine(FeatureStr) + "'"); + // Raise it as a warning for better compatibilities. + return make_error("unrecognized tune feature directive '" + + Twine(FeatureStr) + "'"); } } while (!TFString.empty()); @@ -198,10 +200,9 @@ Error parseTuneFeatureString(StringRef TFString, llvm::set_intersection(PositiveFeatures, NegativeFeatures); if (!Intersection.empty()) { std::string IntersectedStr = join(Intersection, "', '"); - return createStringError(inconvertibleErrorCode(), - "Feature(s) '" + Twine(IntersectedStr) + - "' cannot appear in both " - "positive and negative directives"); + return make_error("Feature(s) '" + Twine(IntersectedStr) + + "' cannot appear in both " + "positive and negative directives"); } // Phase 2: Derive implied features. @@ -228,10 +229,9 @@ Error parseTuneFeatureString(StringRef TFString, Intersection = llvm::set_intersection(PositiveFeatures, NegativeFeatures); if (!Intersection.empty()) { std::string IntersectedStr = join(Intersection, "', '"); - return createStringError(inconvertibleErrorCode(), - "Feature(s) '" + Twine(IntersectedStr) + - "' were implied by both " - "positive and negative directives"); + return make_error("Feature(s) '" + Twine(IntersectedStr) + + "' were implied by both " + "positive and negative directives"); } // Export the result. diff --git a/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp b/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp index e85b08a5df5ff..12383bcc3e61f 100644 --- a/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp +++ b/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp @@ -95,7 +95,10 @@ TEST(RISCVTuneFeature, LegalTuneFeatureStrings) { TEST(RISCVTuneFeature, UnrecognizedTuneFeature) { SmallVector Result; - EXPECT_EQ(toString(RISCV::parseTuneFeatureString("32bit", Result)), + auto Err = RISCV::parseTuneFeatureString("32bit", Result); + // This should be an warning. + EXPECT_TRUE(Err.isA()); + EXPECT_EQ(toString(std::move(Err)), "unrecognized tune feature directive '32bit'"); } From b13750236932112aaab7b14df2be6180d175aa04 Mon Sep 17 00:00:00 2001 From: Min-Yih Hsu Date: Tue, 18 Nov 2025 16:41:34 -0800 Subject: [PATCH 3/6] fixup! Ignore unrecognized feature and move on --- llvm/lib/TargetParser/RISCVTargetParser.cpp | 14 ++++++++++---- .../TargetParser/RISCVTargetParserTest.cpp | 5 +++-- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/llvm/lib/TargetParser/RISCVTargetParser.cpp b/llvm/lib/TargetParser/RISCVTargetParser.cpp index 4914ba798f56d..b2d4872d795db 100644 --- a/llvm/lib/TargetParser/RISCVTargetParser.cpp +++ b/llvm/lib/TargetParser/RISCVTargetParser.cpp @@ -167,6 +167,10 @@ Error parseTuneFeatureString(StringRef TFString, const StringSet<> AllTuneFeatureSet(llvm::from_range, TuneFeatures); using SmallStringSet = SmallSet; + // Do not create ParserWarning right away. Instead, we store the warning + // message until the last moment. + std::string WarningMsg; + TFString = TFString.trim(); // Note: StringSet is not really ergnomic to use in this case here. SmallStringSet PositiveFeatures; @@ -190,9 +194,8 @@ Error parseTuneFeatureString(StringRef TFString, "'"); } } else { - // Raise it as a warning for better compatibilities. - return make_error("unrecognized tune feature directive '" + - Twine(FeatureStr) + "'"); + raw_string_ostream SS(WarningMsg); + SS << "unrecognized tune feature directive '" << FeatureStr << "'"; } } while (!TFString.empty()); @@ -242,7 +245,10 @@ Error parseTuneFeatureString(StringRef TFString, for (StringRef NF : NegativeFeatures) ResFeatures.emplace_back(NegPrefix + NF.str()); - return Error::success(); + if (WarningMsg.empty()) + return Error::success(); + else + return make_error(WarningMsg); } } // namespace RISCV diff --git a/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp b/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp index 12383bcc3e61f..6288601529da2 100644 --- a/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp +++ b/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp @@ -93,13 +93,14 @@ TEST(RISCVTuneFeature, LegalTuneFeatureStrings) { EXPECT_TRUE(is_contained(Result, "-no-default-unroll")); } -TEST(RISCVTuneFeature, UnrecognizedTuneFeature) { +TEST(RISCVTuneFeature, IgnoreUnrecognizedTuneFeature) { SmallVector Result; - auto Err = RISCV::parseTuneFeatureString("32bit", Result); + auto Err = RISCV::parseTuneFeatureString("32bit,log-vrgather", Result); // This should be an warning. EXPECT_TRUE(Err.isA()); EXPECT_EQ(toString(std::move(Err)), "unrecognized tune feature directive '32bit'"); + EXPECT_TRUE(is_contained(Result, "+log-vrgather")); } TEST(RISCVTuneFeature, DuplicatedFeatures) { From be277d08d655b65380b8d67b020b45470fb57b14 Mon Sep 17 00:00:00 2001 From: Min-Yih Hsu Date: Wed, 19 Nov 2025 12:03:45 -0800 Subject: [PATCH 4/6] fixup! Allow users to use custom directive names --- llvm/lib/Target/RISCV/RISCVFeatures.td | 119 +++++++++------- llvm/lib/TargetParser/RISCVTargetParser.cpp | 127 ++++++++++++------ .../TargetParser/RISCVTargetParserTest.cpp | 20 ++- .../TableGen/Basic/RISCVTargetDefEmitter.cpp | 103 ++++++++++---- 4 files changed, 259 insertions(+), 110 deletions(-) diff --git a/llvm/lib/Target/RISCV/RISCVFeatures.td b/llvm/lib/Target/RISCV/RISCVFeatures.td index ef5203445b55c..65594a672c0b4 100644 --- a/llvm/lib/Target/RISCV/RISCVFeatures.td +++ b/llvm/lib/Target/RISCV/RISCVFeatures.td @@ -1740,9 +1740,12 @@ def HasVendorXSMTVDot // LLVM specific features and extensions //===----------------------------------------------------------------------===// -class RISCVTuneFeature implied = []> - : SubtargetFeature; +class RISCVTuneFeatureBase; +class RISCVSimpleTuneFeature : RISCVTuneFeatureBase; +class RISCVTuneFeature : RISCVTuneFeatureBase { + string PositiveDirectiveName = pos_directive; + string NegativeDirectiveName = neg_directive; +} // Feature32Bit exists to mark CPUs that support RV32 to distinguish them from // tuning CPU names. @@ -1792,48 +1795,59 @@ def FeatureUnalignedVectorMem "loads and stores">; def TuneNLogNVRGather - : RISCVTuneFeature<"log-vrgather", "RISCVVRGatherCostModel", "NLog2N", - "Has vrgather.vv with LMUL*log2(LMUL) latency">; + : SubtargetFeature<"log-vrgather", "RISCVVRGatherCostModel", "NLog2N", + "Has vrgather.vv with LMUL*log2(LMUL) latency">, + RISCVSimpleTuneFeature; -def TunePostRAScheduler : RISCVTuneFeature<"use-postra-scheduler", - "UsePostRAScheduler", "true", "Schedule again after register allocation">; +def TunePostRAScheduler : SubtargetFeature<"use-postra-scheduler", + "UsePostRAScheduler", "true", "Schedule again after register allocation">, + RISCVSimpleTuneFeature; -def TuneDisableMISchedLoadClustering : RISCVTuneFeature<"disable-misched-load-clustering", - "EnableMISchedLoadClustering", "false", "Disable load clustering in the machine scheduler">; +def TuneDisableMISchedLoadClustering : SubtargetFeature<"disable-misched-load-clustering", + "EnableMISchedLoadClustering", "false", "Disable load clustering in the machine scheduler">, + RISCVTuneFeature<"disable-misched-load-clustering", "enable-misched-load-clustering">; -def TuneDisableMISchedStoreClustering : RISCVTuneFeature<"disable-misched-store-clustering", - "EnableMISchedStoreClustering", "false", "Disable store clustering in the machine scheduler">; +def TuneDisableMISchedStoreClustering : SubtargetFeature<"disable-misched-store-clustering", + "EnableMISchedStoreClustering", "false", "Disable store clustering in the machine scheduler">, + RISCVTuneFeature<"disable-misched-store-clustering", "enable-misched-store-clustering">; -def TuneDisablePostMISchedLoadClustering : RISCVTuneFeature<"disable-postmisched-load-clustering", - "EnablePostMISchedLoadClustering", "false", "Disable PostRA load clustering in the machine scheduler">; +def TuneDisablePostMISchedLoadClustering : SubtargetFeature<"disable-postmisched-load-clustering", + "EnablePostMISchedLoadClustering", "false", "Disable PostRA load clustering in the machine scheduler">, + RISCVTuneFeature<"disable-postmisched-load-clustering", "enable-postmisched-load-clustering">; -def TuneDisablePostMISchedStoreClustering : RISCVTuneFeature<"disable-postmisched-store-clustering", - "EnablePostMISchedStoreClustering", "false", "Disable PostRA store clustering in the machine scheduler">; +def TuneDisablePostMISchedStoreClustering : SubtargetFeature<"disable-postmisched-store-clustering", + "EnablePostMISchedStoreClustering", "false", "Disable PostRA store clustering in the machine scheduler">, + RISCVTuneFeature<"disable-postmisched-store-clustering", "enable-postmisched-store-clustering">; def TuneDisableLatencySchedHeuristic - : RISCVTuneFeature<"disable-latency-sched-heuristic", "DisableLatencySchedHeuristic", "true", - "Disable latency scheduling heuristic">; + : SubtargetFeature<"disable-latency-sched-heuristic", "DisableLatencySchedHeuristic", "true", + "Disable latency scheduling heuristic">, + RISCVTuneFeature<"disable-latency-sched-heuristic", "enable-latency-sched-heuristic">; def TunePredictableSelectIsExpensive - : RISCVTuneFeature<"predictable-select-expensive", "PredictableSelectIsExpensive", "true", - "Prefer likely predicted branches over selects">; + : SubtargetFeature<"predictable-select-expensive", "PredictableSelectIsExpensive", "true", + "Prefer likely predicted branches over selects">, + RISCVSimpleTuneFeature; def TuneOptimizedZeroStrideLoad - : RISCVTuneFeature<"optimized-zero-stride-load", "HasOptimizedZeroStrideLoad", + : SubtargetFeature<"optimized-zero-stride-load", "HasOptimizedZeroStrideLoad", "true", "Optimized (perform fewer memory operations)" - "zero-stride vector load">; + "zero-stride vector load">, + RISCVSimpleTuneFeature; foreach nf = {2-8} in def TuneOptimizedNF#nf#SegmentLoadStore : - RISCVTuneFeature<"optimized-nf"#nf#"-segment-load-store", + SubtargetFeature<"optimized-nf"#nf#"-segment-load-store", "HasOptimizedNF"#nf#"SegmentLoadStore", "true", "vlseg"#nf#"eN.v and vsseg"#nf#"eN.v are " - "implemented as a wide memory op and shuffle">; + "implemented as a wide memory op and shuffle">, + RISCVSimpleTuneFeature; def TuneVLDependentLatency - : RISCVTuneFeature<"vl-dependent-latency", "HasVLDependentLatency", "true", + : SubtargetFeature<"vl-dependent-latency", "HasVLDependentLatency", "true", "Latency of vector instructions is dependent on the " - "dynamic value of vl">; + "dynamic value of vl">, + RISCVSimpleTuneFeature; def Experimental : SubtargetFeature<"experimental", "HasExperimental", @@ -1843,52 +1857,61 @@ def Experimental // and instead split over multiple cycles. DLEN refers to the datapath width // that can be done in parallel. def TuneDLenFactor2 - : RISCVTuneFeature<"dlen-factor-2", "DLenFactor2", "true", - "Vector unit DLEN(data path width) is half of VLEN">; + : SubtargetFeature<"dlen-factor-2", "DLenFactor2", "true", + "Vector unit DLEN(data path width) is half of VLEN">, + RISCVSimpleTuneFeature; def TuneNoDefaultUnroll - : RISCVTuneFeature<"no-default-unroll", "EnableDefaultUnroll", "false", - "Disable default unroll preference.">; + : SubtargetFeature<"no-default-unroll", "EnableDefaultUnroll", "false", + "Disable default unroll preference.">, + RISCVTuneFeature<"no-default-unroll", "enable-default-unroll">; // SiFive 7 is able to fuse integer ALU operations with a preceding branch // instruction. def TuneShortForwardBranchOpt - : RISCVTuneFeature<"short-forward-branch-opt", "HasShortForwardBranchOpt", - "true", "Enable short forward branch optimization">; + : SubtargetFeature<"short-forward-branch-opt", "HasShortForwardBranchOpt", + "true", "Enable short forward branch optimization">, + RISCVSimpleTuneFeature; def HasShortForwardBranchOpt : Predicate<"Subtarget->hasShortForwardBranchOpt()">; def NoShortForwardBranchOpt : Predicate<"!Subtarget->hasShortForwardBranchOpt()">; def TuneShortForwardBranchIMinMax - : RISCVTuneFeature<"short-forward-branch-i-minmax", "HasShortForwardBranchIMinMax", + : SubtargetFeature<"short-forward-branch-i-minmax", "HasShortForwardBranchIMinMax", "true", "Enable short forward branch optimization for min,max instructions in Zbb", - [TuneShortForwardBranchOpt]>; + [TuneShortForwardBranchOpt]>, + RISCVSimpleTuneFeature; def TuneShortForwardBranchIMul - : RISCVTuneFeature<"short-forward-branch-i-mul", "HasShortForwardBranchIMul", + : SubtargetFeature<"short-forward-branch-i-mul", "HasShortForwardBranchIMul", "true", "Enable short forward branch optimization for mul instruction", - [TuneShortForwardBranchOpt]>; + [TuneShortForwardBranchOpt]>, + RISCVSimpleTuneFeature; // Some subtargets require a S2V transfer buffer to move scalars into vectors. // FIXME: Forming .vx/.vf/.wx/.wf can reduce register pressure. def TuneNoSinkSplatOperands - : RISCVTuneFeature<"no-sink-splat-operands", "SinkSplatOperands", + : SubtargetFeature<"no-sink-splat-operands", "SinkSplatOperands", "false", "Disable sink splat operands to enable .vx, .vf," - ".wx, and .wf instructions">; + ".wx, and .wf instructions">, + RISCVTuneFeature<"no-sink-splat-operands", "sink-splat-operands">; def TunePreferWInst - : RISCVTuneFeature<"prefer-w-inst", "PreferWInst", "true", - "Prefer instructions with W suffix">; + : SubtargetFeature<"prefer-w-inst", "PreferWInst", "true", + "Prefer instructions with W suffix">, + RISCVSimpleTuneFeature; def TuneConditionalCompressedMoveFusion - : RISCVTuneFeature<"conditional-cmv-fusion", "HasConditionalCompressedMoveFusion", - "true", "Enable branch+c.mv fusion">; + : SubtargetFeature<"conditional-cmv-fusion", "HasConditionalCompressedMoveFusion", + "true", "Enable branch+c.mv fusion">, + RISCVSimpleTuneFeature; def HasConditionalMoveFusion : Predicate<"Subtarget->hasConditionalMoveFusion()">; def NoConditionalMoveFusion : Predicate<"!Subtarget->hasConditionalMoveFusion()">; def TuneHasSingleElementVecFP64 - : RISCVTuneFeature<"single-element-vec-fp64", "HasSingleElementVectorFP64", "true", + : SubtargetFeature<"single-element-vec-fp64", "HasSingleElementVectorFP64", "true", "Certain vector FP64 operations produce a single result " - "element per cycle">; + "element per cycle">, + RISCVSimpleTuneFeature; def TuneMIPSP8700 : SubtargetFeature<"mips-p8700", "RISCVProcFamily", "MIPSP8700", @@ -1903,14 +1926,16 @@ def TuneVentanaVeyron : SubtargetFeature<"ventana-veyron", "RISCVProcFamily", "V def TuneAndes45 : SubtargetFeature<"andes45", "RISCVProcFamily", "Andes45", "Andes 45-Series processors">; -def TuneVXRMPipelineFlush : RISCVTuneFeature<"vxrm-pipeline-flush", "HasVXRMPipelineFlush", - "true", "VXRM writes causes pipeline flush">; +def TuneVXRMPipelineFlush : SubtargetFeature<"vxrm-pipeline-flush", "HasVXRMPipelineFlush", + "true", "VXRM writes causes pipeline flush">, + RISCVSimpleTuneFeature; def TunePreferVsetvliOverReadVLENB - : RISCVTuneFeature<"prefer-vsetvli-over-read-vlenb", + : SubtargetFeature<"prefer-vsetvli-over-read-vlenb", "PreferVsetvliOverReadVLENB", "true", - "Prefer vsetvli over read vlenb CSR to calculate VLEN">; + "Prefer vsetvli over read vlenb CSR to calculate VLEN">, + RISCVSimpleTuneFeature; // Assume that lock-free native-width atomics are available, even if the target // and operating system combination would not usually provide them. The user diff --git a/llvm/lib/TargetParser/RISCVTargetParser.cpp b/llvm/lib/TargetParser/RISCVTargetParser.cpp index b2d4872d795db..f8b4c05c83268 100644 --- a/llvm/lib/TargetParser/RISCVTargetParser.cpp +++ b/llvm/lib/TargetParser/RISCVTargetParser.cpp @@ -16,8 +16,8 @@ #include "llvm/ADT/SmallSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringExtras.h" -#include "llvm/ADT/StringSet.h" #include "llvm/ADT/StringSwitch.h" +#include "llvm/ADT/StringTable.h" #include "llvm/TargetParser/RISCVISAInfo.h" namespace llvm { @@ -152,20 +152,83 @@ void getFeaturesForCPU(StringRef CPU, EnabledFeatures.push_back(F.substr(1)); } -using RISCVImpliedTuneFeature = std::pair; +class RISCVTuneFeatureLookupTable { + struct RISCVTuneFeature { + unsigned PosIdx; + unsigned NegIdx; + unsigned FeatureIdx; + }; + + struct RISCVImpliedTuneFeature { + unsigned FeatureIdx; + unsigned ImpliedFeatureIdx; + }; #define GET_TUNE_FEATURES -#define GET_IMPLIED_TUNE_FEATURES #include "llvm/TargetParser/RISCVTargetParserDef.inc" + // Positive directive name -> Feature name + StringMap PositiveMap; + // Negative directive name -> Feature name + StringMap NegativeMap; + + StringMap> ImpliedFeatureMap; + StringMap> InvImpliedFeatureMap; + +public: + static void getAllTuneFeatures(SmallVectorImpl &Features) { + for (const auto &TuneFeature : TuneFeatures) + Features.push_back(TuneFeatureStrings[TuneFeature.FeatureIdx]); + } + + RISCVTuneFeatureLookupTable() { + for (const auto &TuneFeature : TuneFeatures) { + StringRef PosDirective = TuneFeatureStrings[TuneFeature.PosIdx]; + StringRef NegDirective = TuneFeatureStrings[TuneFeature.NegIdx]; + StringRef FeatureName = TuneFeatureStrings[TuneFeature.FeatureIdx]; + PositiveMap[PosDirective] = FeatureName; + NegativeMap[NegDirective] = FeatureName; + } + + for (const auto &Imp : ImpliedTuneFeatures) { + StringRef Feature = TuneFeatureStrings[Imp.FeatureIdx]; + StringRef ImpliedFeature = TuneFeatureStrings[Imp.ImpliedFeatureIdx]; + ImpliedFeatureMap[Feature].push_back(ImpliedFeature); + InvImpliedFeatureMap[ImpliedFeature].push_back(Feature); + } + } + + /// Returns {Feature name, Is positive or not}, or empty feature name + /// if not found. + std::pair getFeature(StringRef DirectiveName) const { + auto It = PositiveMap.find(DirectiveName); + if (It != PositiveMap.end()) + return {It->getValue(), /*IsPositive=*/true}; + + return {NegativeMap.lookup(DirectiveName), /*IsPositive=*/false}; + } + + /// Returns the implied features, or empty ArrayRef if not found. Note: + /// ImpliedFeatureMap / InvImpliedFeatureMap are the owner of these implied + /// feature list, so we can just return the ArrayRef. + ArrayRef featureImplies(StringRef FeatureName, + bool Inverse = false) const { + const auto &Map = Inverse ? InvImpliedFeatureMap : ImpliedFeatureMap; + auto It = Map.find(FeatureName); + if (It == Map.end()) + return {}; + return It->second; + } +}; + void getAllTuneFeatures(SmallVectorImpl &Features) { - Features.assign(std::begin(TuneFeatures), std::end(TuneFeatures)); + RISCVTuneFeatureLookupTable::getAllTuneFeatures(Features); } Error parseTuneFeatureString(StringRef TFString, SmallVectorImpl &ResFeatures) { - const StringSet<> AllTuneFeatureSet(llvm::from_range, TuneFeatures); using SmallStringSet = SmallSet; + RISCVTuneFeatureLookupTable TFLookup; // Do not create ParserWarning right away. Instead, we store the warning // message until the last moment. @@ -176,27 +239,21 @@ Error parseTuneFeatureString(StringRef TFString, SmallStringSet PositiveFeatures; SmallStringSet NegativeFeatures; // Phase 1: Collect explicit features. - StringRef FeatureStr; + StringRef DirectiveStr; do { - std::tie(FeatureStr, TFString) = TFString.split(","); - if (AllTuneFeatureSet.count(FeatureStr)) { - if (!PositiveFeatures.insert(FeatureStr).second) - return make_error( - "cannot specify more than one instance of '" + Twine(FeatureStr) + - "'"); - } else if (FeatureStr.starts_with("no-")) { - // Check if this is a negative feature, like `no-foo` for `foo`. - StringRef ActualFeature = FeatureStr.drop_front(3); - if (AllTuneFeatureSet.count(ActualFeature)) { - if (!NegativeFeatures.insert(ActualFeature).second) - return make_error( - "cannot specify more than one instance of '" + Twine(FeatureStr) + - "'"); - } - } else { + std::tie(DirectiveStr, TFString) = TFString.split(","); + auto [FeatureName, IsPositive] = TFLookup.getFeature(DirectiveStr); + if (FeatureName.empty()) { raw_string_ostream SS(WarningMsg); - SS << "unrecognized tune feature directive '" << FeatureStr << "'"; + SS << "unrecognized tune feature directive '" << DirectiveStr << "'"; + continue; } + + auto &Features = IsPositive ? PositiveFeatures : NegativeFeatures; + if (!Features.insert(FeatureName).second) + return make_error( + "cannot specify more than one instance of '" + Twine(DirectiveStr) + + "'"); } while (!TFString.empty()); auto Intersection = @@ -209,25 +266,19 @@ Error parseTuneFeatureString(StringRef TFString, } // Phase 2: Derive implied features. - StringMap> ImpliedFeatureMap; - StringMap> InverseImpliedFeatureMap; - for (auto [Feature, ImpliedFeature] : ImpliedTuneFeatures) { - ImpliedFeatureMap[Feature].push_back(ImpliedFeature); - InverseImpliedFeatureMap[ImpliedFeature].push_back(Feature); - } - + SmallStringSet DerivedPosFeatures; + SmallStringSet DerivedNegFeatures; for (StringRef PF : PositiveFeatures) { - auto ItFeatures = ImpliedFeatureMap.find(PF); - if (ItFeatures != ImpliedFeatureMap.end()) - PositiveFeatures.insert(ItFeatures->second.begin(), - ItFeatures->second.end()); + if (auto FeatureList = TFLookup.featureImplies(PF); !FeatureList.empty()) + DerivedPosFeatures.insert_range(FeatureList); } for (StringRef NF : NegativeFeatures) { - auto ItFeatures = InverseImpliedFeatureMap.find(NF); - if (ItFeatures != InverseImpliedFeatureMap.end()) - NegativeFeatures.insert(ItFeatures->second.begin(), - ItFeatures->second.end()); + if (auto FeatureList = TFLookup.featureImplies(NF, /*Inverse=*/true); + !FeatureList.empty()) + DerivedNegFeatures.insert_range(FeatureList); } + PositiveFeatures.insert_range(DerivedPosFeatures); + NegativeFeatures.insert_range(DerivedNegFeatures); Intersection = llvm::set_intersection(PositiveFeatures, NegativeFeatures); if (!Intersection.empty()) { diff --git a/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp b/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp index 6288601529da2..d294fee88557f 100644 --- a/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp +++ b/llvm/unittests/TargetParser/RISCVTargetParserTest.cpp @@ -36,6 +36,8 @@ TEST(RISCVVType, CheckSameRatioLMUL) { TEST(RISCVTuneFeature, AllTuneFeatures) { SmallVector AllTuneFeatures; RISCV::getAllTuneFeatures(AllTuneFeatures); + // Only allowed subtarget features that are explicitly marked by + // special TableGen class. EXPECT_EQ(AllTuneFeatures.size(), 28U); for (auto F : {"conditional-cmv-fusion", "dlen-factor-2", @@ -80,6 +82,7 @@ TEST(RISCVTuneFeature, LegalTuneFeatureStrings) { EXPECT_TRUE(is_contained(Result, "-short-forward-branch-i-mul")); Result.clear(); + // Test inverse implied features. EXPECT_FALSE(errorToBool(RISCV::parseTuneFeatureString( "no-short-forward-branch-i-mul,short-forward-branch-i-minmax", Result))); EXPECT_TRUE(is_contained(Result, "+short-forward-branch-i-minmax")); @@ -87,10 +90,14 @@ TEST(RISCVTuneFeature, LegalTuneFeatureStrings) { EXPECT_TRUE(is_contained(Result, "-short-forward-branch-i-mul")); Result.clear(); - EXPECT_FALSE(errorToBool(RISCV::parseTuneFeatureString( - "no-no-default-unroll,no-sink-splat-operands", Result))); + // Test custom directive names. + EXPECT_FALSE(errorToBool( + RISCV::parseTuneFeatureString("enable-default-unroll,no-sink-splat-" + "operands,enable-latency-sched-heuristic", + Result))); EXPECT_TRUE(is_contained(Result, "+no-sink-splat-operands")); EXPECT_TRUE(is_contained(Result, "-no-default-unroll")); + EXPECT_TRUE(is_contained(Result, "-disable-latency-sched-heuristic")); } TEST(RISCVTuneFeature, IgnoreUnrecognizedTuneFeature) { @@ -116,6 +123,15 @@ TEST(RISCVTuneFeature, DuplicatedFeatures) { "Feature(s) 'log-vrgather', 'short-forward-branch-i-mul' cannot " "appear in both positive and negative directives"); + // The error message should show the feature name for those using custom + // directives. + EXPECT_EQ( + toString(RISCV::parseTuneFeatureString( + "disable-latency-sched-heuristic,enable-latency-sched-heuristic", + Result)), + "Feature(s) 'disable-latency-sched-heuristic' cannot appear in both " + "positive and negative directives"); + EXPECT_EQ( toString(RISCV::parseTuneFeatureString( "short-forward-branch-i-mul,no-short-forward-branch-opt", Result)), diff --git a/llvm/utils/TableGen/Basic/RISCVTargetDefEmitter.cpp b/llvm/utils/TableGen/Basic/RISCVTargetDefEmitter.cpp index 7420fa439ecd4..cbebd2577e81f 100644 --- a/llvm/utils/TableGen/Basic/RISCVTargetDefEmitter.cpp +++ b/llvm/utils/TableGen/Basic/RISCVTargetDefEmitter.cpp @@ -13,9 +13,11 @@ #include "llvm/ADT/DenseSet.h" #include "llvm/Support/Format.h" +#include "llvm/Support/FormatVariadic.h" #include "llvm/Support/RISCVISAUtils.h" #include "llvm/TableGen/Error.h" #include "llvm/TableGen/Record.h" +#include "llvm/TableGen/StringToOffsetTable.h" #include "llvm/TableGen/TableGenBackend.h" using namespace llvm; @@ -265,47 +267,102 @@ static void emitRISCVExtensionBitmask(const RecordKeeper &RK, raw_ostream &OS) { static void emitRISCVTuneFeatures(const RecordKeeper &RK, raw_ostream &OS) { std::vector TuneFeatureRecords = - RK.getAllDerivedDefinitionsIfDefined("RISCVTuneFeature"); + RK.getAllDerivedDefinitionsIfDefined("RISCVTuneFeatureBase"); + + // {Post Directive Idx, Neg Directive Idx, TuneFeature Record} + SmallVector> + TuneFeatureDirectives; + // {Directive Idx -> Original Record} + // This is primarily for diagnosing purposes -- when there is a duplication, + // we are able to pointed out the previous definition. + DenseMap DirectiveToRecord; + // A list of {Feature Name, Implied Feature Name} + SmallVector> ImpliedFeatureList; + StringToOffsetTable StrTable; + + auto tryInsertDirectives = [&](StringRef PosName, StringRef NegName, + const Record *R) { + unsigned PosIdx = StrTable.GetOrAddStringOffset(PosName); + if (auto [ItEntry, Inserted] = DirectiveToRecord.try_emplace(PosIdx, R); + !Inserted) { + PrintError(R, "RISC-V tune feature positive directive '" + + Twine(PosName) + "' was already defined"); + PrintFatalNote(ItEntry->second, "Previously defined here"); + } + unsigned NegIdx = StrTable.GetOrAddStringOffset(NegName); + if (auto [ItEntry, Inserted] = DirectiveToRecord.try_emplace(NegIdx, R); + !Inserted) { + PrintError(R, "RISC-V tune feature negative directive '" + + Twine(NegName) + "' was already defined"); + PrintFatalNote(ItEntry->second, "Previously defined here"); + } - SmallVector TuneFeatures; - // A list of {Feature -> Implied Feature} - SmallVector> ImpliedFeatures; + TuneFeatureDirectives.emplace_back(PosIdx, NegIdx, R); + }; + + const std::string SimpleNegPrefix("no-"); for (const auto *R : TuneFeatureRecords) { + if (!R->isSubClassOf("SubtargetFeature")) + PrintFatalError( + R, "A RISC-V tune feature should also be a SubtargetFeature"); + // Preemptively insert feature name into the string table because we know + // it will be used later. StringRef FeatureName = R->getValueAsString("Name"); - TuneFeatures.push_back(FeatureName); + StrTable.GetOrAddStringOffset(FeatureName); + if (R->isSubClassOf("RISCVSimpleTuneFeature")) { + // The positive directve will be the feature name, and the negative + // directive will be "no-" + feature name. + std::string NegName = SimpleNegPrefix + FeatureName.str(); + tryInsertDirectives(FeatureName, NegName, R); + } else if (R->isSubClassOf("RISCVTuneFeature")) { + StringRef PosName = R->getValueAsString("PositiveDirectiveName"); + StringRef NegName = R->getValueAsString("NegativeDirectiveName"); + tryInsertDirectives(PosName, NegName, R); + } else { + llvm_unreachable("unrecognized RISCVTuneFeatureBase"); + } + } + + for (const auto *R : TuneFeatureRecords) { std::vector Implies = R->getValueAsListOfDefs("Implies"); for (const auto *ImpliedRecord : Implies) { - if (!ImpliedRecord->isSubClassOf("RISCVTuneFeature") || + if (!ImpliedRecord->isSubClassOf("RISCVTuneFeatureBase") || ImpliedRecord == R) { PrintError(ImpliedRecord, - "RISCVTuneFeature can only imply other RISCVTuneFeatures"); - PrintFatalNote(R, "implied by this RISCVTuneFeature"); + "A RISC-V tune feature can only imply another tune feature"); + PrintFatalNote(R, "implied by this tune feature"); } - ImpliedFeatures.emplace_back(FeatureName, - ImpliedRecord->getValueAsString("Name")); + StringRef CurrFeatureName = R->getValueAsString("Name"); + StringRef ImpliedFeatureName = ImpliedRecord->getValueAsString("Name"); + + ImpliedFeatureList.emplace_back(CurrFeatureName, ImpliedFeatureName); } } OS << "#ifdef GET_TUNE_FEATURES\n"; OS << "#undef GET_TUNE_FEATURES\n\n"; - OS << "static const char *TuneFeatures[] = {\n"; - for (StringRef Feature : TuneFeatures) - OS.indent(4) << "\"" << Feature << "\",\n"; - OS << "};\n\n"; - - OS << "#endif // GET_TUNE_FEATURES\n\n"; + StrTable.EmitStringTableDef(OS, "TuneFeatureStrings"); + OS << "\n"; - OS << "#ifdef GET_IMPLIED_TUNE_FEATURES\n"; - OS << "#undef GET_IMPLIED_TUNE_FEATURES\n\n"; + OS << "static constexpr RISCVTuneFeature TuneFeatures[] = {\n"; + for (const auto &[PosIdx, NegIdx, R] : TuneFeatureDirectives) { + StringRef FeatureName = R->getValueAsString("Name"); + OS.indent(4) << formatv("{{ {0}, {1}, {2} },\t// '{3}'\n", PosIdx, NegIdx, + *StrTable.GetStringOffset(FeatureName), + FeatureName); + } + OS << "};\n\n"; - OS << "static const RISCVImpliedTuneFeature ImpliedTuneFeatures[] = {\n"; - for (auto [Feature, ImpliedFeature] : ImpliedFeatures) - OS.indent(4) << "{" << "\"" << Feature << "\", \"" << ImpliedFeature - << "\"},\n"; + OS << "static constexpr RISCVImpliedTuneFeature ImpliedTuneFeatures[] = {\n"; + for (auto [Feature, ImpliedFeature] : ImpliedFeatureList) + OS.indent(4) << formatv("{{ {0}, {1} }, // '{2}' -> '{3}'\n", + *StrTable.GetStringOffset(Feature), + *StrTable.GetStringOffset(ImpliedFeature), Feature, + ImpliedFeature); OS << "};\n\n"; - OS << "#endif // GET_IMPLIED_TUNE_FEATURES\n"; + OS << "#endif // GET_TUNE_FEATURES\n"; } static void emitRiscvTargetDef(const RecordKeeper &RK, raw_ostream &OS) { From bc3e3ca4970c0141b8151b71caf19a1935b69818 Mon Sep 17 00:00:00 2001 From: Min-Yih Hsu Date: Wed, 19 Nov 2025 13:09:03 -0800 Subject: [PATCH 5/6] fixup! Add TableGen tests --- llvm/test/TableGen/riscv-target-def.td | 35 ++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/llvm/test/TableGen/riscv-target-def.td b/llvm/test/TableGen/riscv-target-def.td index 79178731f12a7..d12fe0a1681bb 100644 --- a/llvm/test/TableGen/riscv-target-def.td +++ b/llvm/test/TableGen/riscv-target-def.td @@ -1,4 +1,6 @@ // RUN: llvm-tblgen -gen-riscv-target-def -I %p/../../include %s | FileCheck %s +// RUN: not llvm-tblgen -gen-riscv-target-def -DINVALID_IMPLY -I %p/../../include %s 2>&1 | FileCheck %s --check-prefix=INVALID-IMPLY +// RUN: not llvm-tblgen -gen-riscv-target-def -DDUPLICATE -I %p/../../include %s 2>&1 | FileCheck %s --check-prefix=DUPLICATE include "llvm/Target/Target.td" @@ -26,6 +28,13 @@ class RISCVExperimentalExtension : RISCVTuneFeatureBase { + string PositiveDirectiveName = pos_directive; + string NegativeDirectiveName = neg_directive; +} + def FeatureStdExtI : RISCVExtension<"i", 2, 1, "'I' (Base Integer Instruction Set)">, @@ -188,3 +197,29 @@ def ROCKET : RISCVTuneProcessorModel<"rocket", // CHECK-NEXT: {"i", 0, 8ULL}, // CHECK-NEXT: }; // CHECK-NEXT: #endif + +#ifdef INVALID_IMPLY +def TuneFeatureBar : SubtargetFeature<"bar", "HasBar", "true", "TBA">; + +def TuneFeatureFoo : SubtargetFeature<"foo", "HasFoo", "true", "TBA", + [TuneFeatureBar]>, + RISCVSimpleTuneFeature; + +// INVALID-IMPLY: error: A RISC-V tune feature can only imply another tune feature +// INVALID-IMPLY-NEXT: def TuneFeatureBar +// INVALID-IMPLY: note: implied by this tune feature +// INVALID-IMPLY-NEXT: def TuneFeatureFoo +#endif + +#ifdef DUPLICATE +def TuneFeatureBar : SubtargetFeature<"bar", "HasBar", "true", "TBA">, + RISCVTuneFeature<"abc", "def">; + +def TuneFeatureFoo : SubtargetFeature<"foo", "HasFoo", "true", "TBA">, + RISCVTuneFeature<"xyz", "abc">; + +// DUPLICATE: error: RISC-V tune feature negative directive 'abc' was already defined +// DUPLICATE-NEXT: def TuneFeatureFoo +// DUPLICATE: note: Previously defined here +// DUPLICATE-NEXT: def TuneFeatureBar +#endif From 9b5dc6e44b2b7a71d32e31271211fff2e626dc54 Mon Sep 17 00:00:00 2001 From: Min-Yih Hsu Date: Thu, 20 Nov 2025 09:04:38 -0800 Subject: [PATCH 6/6] fixup! Minor changes --- llvm/lib/TargetParser/RISCVTargetParser.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/llvm/lib/TargetParser/RISCVTargetParser.cpp b/llvm/lib/TargetParser/RISCVTargetParser.cpp index f8b4c05c83268..e06aa7f068a34 100644 --- a/llvm/lib/TargetParser/RISCVTargetParser.cpp +++ b/llvm/lib/TargetParser/RISCVTargetParser.cpp @@ -152,6 +152,7 @@ void getFeaturesForCPU(StringRef CPU, EnabledFeatures.push_back(F.substr(1)); } +namespace { class RISCVTuneFeatureLookupTable { struct RISCVTuneFeature { unsigned PosIdx; @@ -209,8 +210,8 @@ class RISCVTuneFeatureLookupTable { } /// Returns the implied features, or empty ArrayRef if not found. Note: - /// ImpliedFeatureMap / InvImpliedFeatureMap are the owner of these implied - /// feature list, so we can just return the ArrayRef. + /// ImpliedFeatureMap / InvImpliedFeatureMap are the owners of these implied + /// feature lists, so we can just return the ArrayRef. ArrayRef featureImplies(StringRef FeatureName, bool Inverse = false) const { const auto &Map = Inverse ? InvImpliedFeatureMap : ImpliedFeatureMap; @@ -220,6 +221,7 @@ class RISCVTuneFeatureLookupTable { return It->second; } }; +} // namespace void getAllTuneFeatures(SmallVectorImpl &Features) { RISCVTuneFeatureLookupTable::getAllTuneFeatures(Features); @@ -235,7 +237,7 @@ Error parseTuneFeatureString(StringRef TFString, std::string WarningMsg; TFString = TFString.trim(); - // Note: StringSet is not really ergnomic to use in this case here. + // Note: StringSet is not really ergonomic to use in this case here. SmallStringSet PositiveFeatures; SmallStringSet NegativeFeatures; // Phase 1: Collect explicit features.