From fba58736a1159b3799a90cf15fcd82a8b64285a5 Mon Sep 17 00:00:00 2001 From: pvanhout Date: Tue, 14 May 2024 15:25:38 +0200 Subject: [PATCH] [GlobalISel] Optimized MatchData Handling Surprisingly, MatchDatas were very, very expensive in GlobalISel Combiners. The time spent on constructing the big MatchData struct, and copying it to reset it (`operator=`) was equal or greater than the time spent iterating the MatchTable itself and executing it (excluding the time spent in callees, of course). This was because I put every MatchData we could possibly need in a big struct, and rebuilt that struct every instruction we looked at. Turns out that when all your function is doing is iterating over a byte array and executing opcodes, the time spent allocating and initializing a huge non-trivially constructible/destructible struct is very, very significant. Now, we use a simpler and more efficient strategy: be as lazy as possible, only allocate and deallocate as needed, end of story. The backend doesn't think about it anymore, and we simply don't pay for what we don't use. --- .../llvm/CodeGen/GlobalISel/Combiner.h | 50 +++++++++ .../GlobalISelCombinerEmitter/match-table.td | 23 ++-- .../pattern-parsing.td | 8 +- llvm/utils/TableGen/Common/CMakeLists.txt | 1 - .../Common/GlobalISel/MatchDataInfo.cpp | 49 -------- .../Common/GlobalISel/MatchDataInfo.h | 90 --------------- .../TableGen/GlobalISelCombinerEmitter.cpp | 106 +++++++++++++----- 7 files changed, 144 insertions(+), 183 deletions(-) delete mode 100644 llvm/utils/TableGen/Common/GlobalISel/MatchDataInfo.cpp delete mode 100644 llvm/utils/TableGen/Common/GlobalISel/MatchDataInfo.h diff --git a/llvm/include/llvm/CodeGen/GlobalISel/Combiner.h b/llvm/include/llvm/CodeGen/GlobalISel/Combiner.h index f826601544932..c3b483bdc200f 100644 --- a/llvm/include/llvm/CodeGen/GlobalISel/Combiner.h +++ b/llvm/include/llvm/CodeGen/GlobalISel/Combiner.h @@ -60,6 +60,56 @@ class Combiner : public GIMatchTableExecutor { bool combineMachineInstrs(); + /// Base class for all dynamic MatchDatas definitions, used for type erasure + /// purposes. + /// + /// As derived classes may have non-trivial destructors, we need to be able to + /// call the destructor through the unique_ptr of the base class. + /// + /// There are two ways to achieve this. + /// - (Easiest) Make MatchDataBase have a virtual destructor. + /// - Use a custom deleter + /// + /// We use solution number 2, that way we don't have a vtable in all MatchData + /// objects (making them more compact), and we can have trivially destructible + /// MatchDatas, which avoids useless virtual function calls and allows the + /// compiler/libraries to use faster code (as it knows no destructor needs to + /// be called). + /// + /// This is really a micro-optimization, but MatchData used to be a + /// performance issue so better safe than sorry. + struct MatchDataBase {}; + + /// If a MatchData instance is active, cast it to Ty and return it. + /// Otherwise, create a new MatchData instance of type Ty and return it. + template Ty &getOrCreateMatchData() const { + static_assert(std::is_base_of_v, + "Type must derive from MatchDataBase to be allocatable!"); + // Allocate if we have no MatchData active, or if the MatchData that's + // active is not the one that we want. + if (!MatchData || Ty::ID != MatchDataID) { + MatchDataID = Ty::ID; + MatchData = std::unique_ptr( + new Ty(), [](MatchDataBase *MD) { delete static_cast(MD); }); + } + return *static_cast(MatchData.get()); + } + + /// Deallocates the currently active MatchData instance. + /// + /// TODO: Can we get away with never actually calling this? The MatchData + /// instance would then just be deleted by getOrCreateMatchData or when the + /// Combiner class is deallocated. I'm just a bit scared it'd cause issues + /// with captured values in some of the lambdas used as MatchInfo. + void resetMatchData() const { MatchData.reset(); } + +private: + using MatchDataDeleter = void (*)(MatchDataBase *); + + mutable std::unique_ptr MatchData = { + nullptr, [](auto *) {}}; + mutable unsigned MatchDataID = unsigned(-1); + protected: CombinerInfo &CInfo; GISelChangeObserver &Observer; diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td index 1052e31b2d051..837f4c76c0f18 100644 --- a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td +++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td @@ -76,8 +76,15 @@ def MyCombiner: GICombiner<"GenMyCombiner", [ // We have at most 2 registers used by one rule at a time, so we should only have 2 registers MDInfos. -// CHECK: struct MatchInfosTy { -// CHECK-NEXT: Register MDInfo0, MDInfo1; +// CHECK: struct MatchDataRule2 : MatchDataBase { +// CHECK-NEXT: static constexpr unsigned ID = 2; +// CHECK-NEXT: Register r0; +// CHECK-NEXT: Register r1; +// CHECK-NEXT: }; + +// CHECK: struct MatchDataRule3 : MatchDataBase { +// CHECK-NEXT: static constexpr unsigned ID = 3; +// CHECK-NEXT: Register r0; // CHECK-NEXT: }; // Check predicates @@ -96,13 +103,9 @@ def MyCombiner: GICombiner<"GenMyCombiner", [ // CHECK-NEXT: B.setInstrAndDebugLoc(I); // CHECK-NEXT: State.MIs.clear(); // CHECK-NEXT: State.MIs.push_back(&I); -// CHECK-NEXT: MatchInfos = MatchInfosTy(); -// CHECK-EMPTY: -// CHECK-NEXT: if (executeMatchTable(*this, State, ExecInfo, B, getMatchTable(), *ST.getInstrInfo(), MRI, *MRI.getTargetRegisterInfo(), *ST.getRegBankInfo(), AvailableFeatures, /*CoverageInfo*/ nullptr)) -// CHECK-NEXT: return true; -// CHECK-NEXT: } -// CHECK-EMPTY: -// CHECK-NEXT: return false; +// CHECK-NEXT: const bool Result = executeMatchTable(*this, State, ExecInfo, B, getMatchTable(), *ST.getInstrInfo(), MRI, *MRI.getTargetRegisterInfo(), *ST.getRegBankInfo(), AvailableFeatures, /*CoverageInfo*/ nullptr); +// CHECK-NEXT: resetMatchData(); +// CHECK-NEXT: return Result; // CHECK-NEXT: } // Check apply @@ -120,7 +123,7 @@ def MyCombiner: GICombiner<"GenMyCombiner", [ // CHECK-NEXT: } // CHECK-NEXT: case GICXXCustomAction_CombineApplyGICombiner1:{ // CHECK-NEXT: Helper.getBuilder().setInstrAndDebugLoc(*State.MIs[0]); -// CHECK-NEXT: APPLY MatchInfos.MDInfo0, MatchInfos.MDInfo1 +// CHECK-NEXT: APPLY getOrCreateMatchData().r0, getOrCreateMatchData().r1 // CHECK-NEXT: return; // CHECK-NEXT: } // CHECK-NEXT: case GICXXCustomAction_CombineApplyGICombiner2:{ diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/pattern-parsing.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/pattern-parsing.td index c261ec4575453..3de2924608faa 100644 --- a/llvm/test/TableGen/GlobalISelCombinerEmitter/pattern-parsing.td +++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/pattern-parsing.td @@ -64,8 +64,8 @@ def InstTest0 : GICombineRule< (apply [{ APPLY }])>; // CHECK: (CombineRule name:InstTest1 id:3 root:d -// CHECK-NEXT: (MatchDatas -// CHECK-NEXT: (MatchDataInfo pattern_symbol:r0 type:'Register' var_name:MDInfo0) +// CHECK-NEXT: (MatchData typename:MatchDataRule3 id:3 +// CHECK-NEXT: (MatchDataDef symbol:r0 type:Register) // CHECK-NEXT: ) // CHECK-NEXT: (MatchPats // CHECK-NEXT: d:(CodeGenInstructionPattern COPY operands:[$a, i32:$b]) @@ -89,8 +89,8 @@ def InstTest1 : GICombineRule< (apply [{ APPLY }])>; // CHECK: (CombineRule name:InstTest2 id:4 root:d -// CHECK-NEXT: (MatchDatas -// CHECK-NEXT: (MatchDataInfo pattern_symbol:r0 type:'Register' var_name:MDInfo0) +// CHECK-NEXT: (MatchData typename:MatchDataRule4 id:4 +// CHECK-NEXT: (MatchDataDef symbol:r0 type:Register) // CHECK-NEXT: ) // CHECK-NEXT: (MatchPats // CHECK-NEXT: __InstTest2_match_0:(CodeGenInstructionPattern COPY operands:[$d, (i32 0):$x]) diff --git a/llvm/utils/TableGen/Common/CMakeLists.txt b/llvm/utils/TableGen/Common/CMakeLists.txt index c31ed5a1de690..30f188ae48a2a 100644 --- a/llvm/utils/TableGen/Common/CMakeLists.txt +++ b/llvm/utils/TableGen/Common/CMakeLists.txt @@ -16,7 +16,6 @@ add_llvm_library(LLVMTableGenCommon STATIC OBJECT EXCLUDE_FROM_ALL GlobalISel/CXXPredicates.cpp GlobalISel/GlobalISelMatchTable.cpp GlobalISel/GlobalISelMatchTableExecutorEmitter.cpp - GlobalISel/MatchDataInfo.cpp GlobalISel/PatternParser.cpp GlobalISel/Patterns.cpp diff --git a/llvm/utils/TableGen/Common/GlobalISel/MatchDataInfo.cpp b/llvm/utils/TableGen/Common/GlobalISel/MatchDataInfo.cpp deleted file mode 100644 index b5c9e4f8c2485..0000000000000 --- a/llvm/utils/TableGen/Common/GlobalISel/MatchDataInfo.cpp +++ /dev/null @@ -1,49 +0,0 @@ -//===- MatchDataInfo.cpp ----------------------------------------*- C++ -*-===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// -// -// -//===----------------------------------------------------------------------===// - -#include "MatchDataInfo.h" -#include "llvm/Support/Debug.h" -#include "llvm/Support/raw_ostream.h" - -namespace llvm { -namespace gi { - -StringMap> AllMatchDataVars; - -StringRef MatchDataInfo::getVariableName() const { - assert(hasVariableName()); - return VarName; -} - -void MatchDataInfo::print(raw_ostream &OS) const { - OS << "(MatchDataInfo pattern_symbol:" << PatternSymbol << " type:'" << Type - << "' var_name:" << (VarName.empty() ? "" : VarName) << ")"; -} - -void MatchDataInfo::dump() const { print(dbgs()); } - -void AssignMatchDataVariables(MutableArrayRef Infos) { - static unsigned NextVarID = 0; - - StringMap SeenTypes; - for (auto &Info : Infos) { - unsigned &NumSeen = SeenTypes[Info.getType()]; - auto &ExistingVars = AllMatchDataVars[Info.getType()]; - - if (NumSeen == ExistingVars.size()) - ExistingVars.push_back("MDInfo" + std::to_string(NextVarID++)); - - Info.setVariableName(ExistingVars[NumSeen++]); - } -} - -} // namespace gi -} // namespace llvm diff --git a/llvm/utils/TableGen/Common/GlobalISel/MatchDataInfo.h b/llvm/utils/TableGen/Common/GlobalISel/MatchDataInfo.h deleted file mode 100644 index abe1245bc67d0..0000000000000 --- a/llvm/utils/TableGen/Common/GlobalISel/MatchDataInfo.h +++ /dev/null @@ -1,90 +0,0 @@ -//===- MatchDataInfo.h ------------------------------------------*- C++ -*-===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// -// -/// \file Contains utilities related to handling "match data" for GlobalISel -/// Combiners. Match data allows for setting some arbitrary data in the "match" -/// phase and pass it down to the "apply" phase. -// -//===----------------------------------------------------------------------===// - -#ifndef LLVM_UTILS_MIRPATTERNS_MATCHDATAINFO_H -#define LLVM_UTILS_MIRPATTERNS_MATCHDATAINFO_H - -#include "llvm/ADT/ArrayRef.h" -#include "llvm/ADT/StringMap.h" -#include "llvm/ADT/StringRef.h" -#include -#include - -namespace llvm { - -class raw_ostream; - -namespace gi { - -/// Represents MatchData defined by the match stage and required by the apply -/// stage. -/// -/// This allows the plumbing of arbitrary data from C++ predicates between the -/// stages. -/// -/// When this class is initially created, it only has a pattern symbol and a -/// type. When all of the MatchDatas declarations of a given pattern have been -/// parsed, `AssignVariables` must be called to assign storage variable names to -/// each MatchDataInfo. -class MatchDataInfo { - StringRef PatternSymbol; - StringRef Type; - std::string VarName; - -public: - static constexpr StringLiteral StructTypeName = "MatchInfosTy"; - static constexpr StringLiteral StructName = "MatchInfos"; - - MatchDataInfo(StringRef PatternSymbol, StringRef Type) - : PatternSymbol(PatternSymbol), Type(Type.trim()) {} - - StringRef getPatternSymbol() const { return PatternSymbol; }; - StringRef getType() const { return Type; }; - - bool hasVariableName() const { return !VarName.empty(); } - void setVariableName(StringRef Name) { VarName = Name; } - StringRef getVariableName() const; - - std::string getQualifiedVariableName() const { - return StructName.str() + "." + getVariableName().str(); - } - - void print(raw_ostream &OS) const; - void dump() const; -}; - -/// Pool of type -> variables used to emit MatchData variables declarations. -/// -/// e.g. if the map contains "int64_t" -> ["MD0", "MD1"], then two variable -/// declarations must be emitted: `int64_t MD0` and `int64_t MD1`. -/// -/// This has a static lifetime and will outlive all the `MatchDataInfo` objects -/// by design. It needs a static lifetime so the backends can emit variable -/// declarations after processing all the inputs. -extern StringMap> AllMatchDataVars; - -/// Assign variable names to all MatchDatas used by a pattern. This must be -/// called after all MatchData decls have been parsed for a given processing -/// unit (e.g. a combine rule) -/// -/// Requires an array of MatchDataInfo so we can handle cases where a pattern -/// uses multiple instances of the same MatchData type. -/// -/// Writes to \ref AllMatchDataVars. -void AssignMatchDataVariables(MutableArrayRef Infos); - -} // namespace gi -} // end namespace llvm - -#endif // ifndef LLVM_UTILS_MIRPATTERNS_MATCHDATAINFO_H diff --git a/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp b/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp index ef9e9ff04f85f..02f84bd9f02df 100644 --- a/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp +++ b/llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp @@ -35,7 +35,6 @@ #include "Common/GlobalISel/CombinerUtils.h" #include "Common/GlobalISel/GlobalISelMatchTable.h" #include "Common/GlobalISel/GlobalISelMatchTableExecutorEmitter.h" -#include "Common/GlobalISel/MatchDataInfo.h" #include "Common/GlobalISel/PatternParser.h" #include "Common/GlobalISel/Patterns.h" #include "Common/SubtargetFeatureInfo.h" @@ -605,6 +604,56 @@ CombineRuleOperandTypeChecker::getRuleEqClasses() const { return TECs; } +//===- MatchData Handling -------------------------------------------------===// +// +/// Parsing +/// Record all MatchData definitions seen. +/// +/// Once all defs have been parsed, handleRuleMatchDataDefs is used to create +/// a new MatchDataStruct (if at least one MatchDataDef exists) and add it to +/// AllMatchDataStructDecls for emission later. +/// +/// Layout +/// Each rule that has at least one MatchData will create a struct (See +/// getMatchDataStructTypeName) that derives from Combiner::MatchDataBase +/// and containing each MatchData definition. Fields in that struct are +/// named after the MatchData symbols. +/// +/// Access/Code Expansion +/// Combiner::getOrCreateMatchData is used to fetch (and lazily allocate) +/// MatchDatas as they're needed. +struct MatchDataDef { + MatchDataDef(StringRef Symbol, StringRef Type) : Symbol(Symbol), Type(Type) {} + + StringRef Symbol; + StringRef Type; +}; + +struct MatchDataStruct { + std::string TypeName; + unsigned ID; + std::vector Fields; +}; + +static std::vector> AllMatchDataStructs; + +/// If \p Defs has at least one item, prepare a new MatchDataStruct to contain +/// this rule's MatchData defs. If \p Defs is empty, return nullptr. +static const MatchDataStruct * +handleRuleMatchDataDefs(ArrayRef Defs, unsigned RuleID) { + if (Defs.empty()) + return nullptr; + + auto MDS = std::make_unique(); + MDS->TypeName = "MatchDataRule" + std::to_string(RuleID); + MDS->ID = RuleID; + for (const auto &Def : Defs) + MDS->Fields.push_back(Def); + auto *Ptr = MDS.get(); + AllMatchDataStructs.push_back(std::move(MDS)); + return Ptr; +} + //===- CombineRuleBuilder -------------------------------------------------===// /// Parses combine rule and builds a small intermediate representation to tie @@ -777,8 +826,8 @@ class CombineRuleBuilder { Pattern *MatchRoot = nullptr; SmallDenseSet ApplyRoots; - SmallVector MatchDatas; SmallVector PermutationsToEmit; + const MatchDataStruct *RuleMatchData = nullptr; }; bool CombineRuleBuilder::parseAll() { @@ -842,13 +891,12 @@ void CombineRuleBuilder::print(raw_ostream &OS) const { OS << "(CombineRule name:" << RuleDef.getName() << " id:" << RuleID << " root:" << RootName << '\n'; - if (!MatchDatas.empty()) { - OS << " (MatchDatas\n"; - for (const auto &MD : MatchDatas) { - OS << " "; - MD.print(OS); - OS << '\n'; - } + if (RuleMatchData) { + OS << " (MatchData typename:" << RuleMatchData->TypeName + << " id:" << RuleMatchData->ID << "\n"; + for (const auto &MD : RuleMatchData->Fields) + OS << " (MatchDataDef symbol:" << MD.Symbol << " type:" << MD.Type + << ")\n"; OS << " )\n"; } @@ -1007,8 +1055,13 @@ bool CombineRuleBuilder::addMatchPattern(std::unique_ptr Pat) { void CombineRuleBuilder::declareAllMatchDatasExpansions( CodeExpansions &CE) const { - for (const auto &MD : MatchDatas) - CE.declare(MD.getPatternSymbol(), MD.getQualifiedVariableName()); + if (!RuleMatchData) + return; + + const std::string BaseAccessor = + "getOrCreateMatchData<" + RuleMatchData->TypeName + ">()."; + for (const auto &MD : RuleMatchData->Fields) + CE.declare(MD.Symbol, (BaseAccessor + MD.Symbol).str()); } void CombineRuleBuilder::addCXXPredicate(RuleMatcher &M, @@ -1429,6 +1482,7 @@ bool CombineRuleBuilder::parseDefs(const DagInit &Def) { return false; } + SmallVector MatchDatas; SmallVector Roots; for (unsigned I = 0, E = Def.getNumArgs(); I < E; ++I) { if (isSpecificDef(*Def.getArg(I), "root")) { @@ -1463,9 +1517,7 @@ bool CombineRuleBuilder::parseDefs(const DagInit &Def) { } RootName = Roots.front(); - - // Assign variables to all MatchDatas. - AssignMatchDataVariables(MatchDatas); + RuleMatchData = handleRuleMatchDataDefs(MatchDatas, RuleID); return true; } @@ -2371,15 +2423,12 @@ void GICombinerEmitter::emitAdditionalImpl(raw_ostream &OS) { << " B.setInstrAndDebugLoc(I);\n" << " State.MIs.clear();\n" << " State.MIs.push_back(&I);\n" - << " " << MatchDataInfo::StructName << " = " - << MatchDataInfo::StructTypeName << "();\n\n" - << " if (executeMatchTable(*this, State, ExecInfo, B" + << " const bool Result = executeMatchTable(*this, State, ExecInfo, B" << ", getMatchTable(), *ST.getInstrInfo(), MRI, " "*MRI.getTargetRegisterInfo(), *ST.getRegBankInfo(), AvailableFeatures" - << ", /*CoverageInfo*/ nullptr)) {\n" - << " return true;\n" - << " }\n\n" - << " return false;\n" + << ", /*CoverageInfo*/ nullptr);\n" + << " resetMatchData();\n" + << " return Result;\n" << "}\n\n"; } @@ -2472,14 +2521,13 @@ void GICombinerEmitter::emitRunCustomAction(raw_ostream &OS) { void GICombinerEmitter::emitAdditionalTemporariesDecl(raw_ostream &OS, StringRef Indent) { - OS << Indent << "struct " << MatchDataInfo::StructTypeName << " {\n"; - for (const auto &[Type, VarNames] : AllMatchDataVars) { - assert(!VarNames.empty() && "Cannot have no vars for this type!"); - OS << Indent << " " << Type << " " << join(VarNames, ", ") << ";\n"; - } - OS << Indent << "};\n" - << Indent << "mutable " << MatchDataInfo::StructTypeName << " " - << MatchDataInfo::StructName << ";\n\n"; + for (const auto &MDS : AllMatchDataStructs) { + OS << Indent << "struct " << MDS->TypeName << " : MatchDataBase {\n" + << Indent << " static constexpr unsigned ID = " << MDS->ID << ";\n"; + for (const auto &Field : MDS->Fields) + OS << Indent << " " << Field.Type << " " << Field.Symbol << ";\n"; + OS << Indent << "};\n\n"; + } } GICombinerEmitter::GICombinerEmitter(RecordKeeper &RK,