diff --git a/llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-parsing.td b/llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-parsing.td --- a/llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-parsing.td +++ b/llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-parsing.td @@ -81,16 +81,16 @@ // CHECK-NEXT: (MatchDataInfo pattern_symbol:r0 type:'Register' var_name:MDInfo0) // CHECK-NEXT: ) // CHECK-NEXT: (MatchPats -// CHECK-NEXT: __anon_pat_match_3_0:(InstructionPattern inst:ZEXT operands:[x, a]) // CHECK-NEXT: d:(InstructionPattern inst:MOV operands:[a, b]) +// CHECK-NEXT: __anon_pat_match_3_0:(InstructionPattern inst:ZEXT operands:[x, a]) // CHECK-NEXT: ) // CHECK-NEXT: (ApplyPats // CHECK-NEXT: __anon_pat_apply_3_1:(CXXPattern apply code:"APPLY") // CHECK-NEXT: ) // CHECK-NEXT: (OperandTable -// CHECK-NEXT: [x match_pat:__anon_pat_match_3_0] // CHECK-NEXT: [a match_pat:d] // CHECK-NEXT: [b live-in] +// CHECK-NEXT: [x match_pat:__anon_pat_match_3_0] // CHECK-NEXT: ) // CHECK-NEXT: ) let Predicates = [HasAnswerToEverything] in diff --git a/llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp b/llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp --- a/llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp +++ b/llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp @@ -233,6 +233,10 @@ private: unsigned Kind; + + // Note: if this ever changes to a StringRef (e.g. allocated in a pool or + // something), CombineRuleBuilder::verify() needs to be updated as well. + // It currently checks that the StringRef in the PatternMap references this. std::string Name; }; @@ -454,12 +458,12 @@ /// - Creation in a `parse` function /// - The unique_ptr is stored in a variable, and may be destroyed if the /// pattern is found to be semantically invalid. -/// - Ownership transfer into a `PatternStringMap` +/// - Ownership transfer into a `PatternMap` /// - Once a pattern is moved into either the map of Match or Apply /// patterns, it is known to be valid and it never moves back. class CombineRuleBuilder { public: - using PatternStringMap = StringMap>; + using PatternMap = MapVector>; CombineRuleBuilder(const CodeGenTarget &CGT, SubtargetFeatureInfoMap &SubtargetFeatures, @@ -547,8 +551,8 @@ /// These maps have ownership of the actual Pattern objects. /// They both map a Pattern's name to the Pattern instance. - PatternStringMap MatchPats; - PatternStringMap ApplyPats; + PatternMap MatchPats; + PatternMap ApplyPats; /// Set by findRoots. Pattern *MatchRoot = nullptr; @@ -614,7 +618,7 @@ OS << " )\n"; } - const auto DumpPats = [&](StringRef Name, const PatternStringMap &Pats) { + const auto DumpPats = [&](StringRef Name, const PatternMap &Pats) { OS << " (" << Name << " "; if (Pats.empty()) { OS << ")\n"; @@ -622,12 +626,12 @@ } OS << "\n"; - for (const auto &P : Pats) { + for (const auto &[Name, Pat] : Pats) { OS << " "; - if (P.getValue().get() == MatchRoot) + if (Pat.get() == MatchRoot) OS << ""; - OS << P.getKey() << ":"; - P.getValue()->print(OS, /*PrintName=*/false); + OS << Name << ":"; + Pat->print(OS, /*PrintName=*/false); OS << "\n"; } OS << " )\n"; @@ -658,15 +662,27 @@ } void CombineRuleBuilder::verify() const { - const auto VerifyPats = [&](const PatternStringMap &Pats) { - for (const auto &Entry : Pats) { - if (!Entry.getValue()) + const auto VerifyPats = [&](const PatternMap &Pats) { + for (const auto &[Name, Pat] : Pats) { + if (!Pat) PrintFatalError("null pattern in pattern map!"); - if (Entry.getKey() != Entry.getValue()->getName()) { - Entry.getValue()->dump(); - PrintFatalError("Pattern name mismatch! Map name: " + Entry.getKey() + - ", Pat name: " + Entry.getValue()->getName()); + if (Name != Pat->getName()) { + Pat->dump(); + PrintFatalError("Pattern name mismatch! Map name: " + Name + + ", Pat name: " + Pat->getName()); + } + + // As an optimization, the PatternMaps don't re-allocate the PatternName + // string. They simply reference the std::string inside Pattern. Ensure + // this is the case to avoid memory issues. + if (Name.data() != Pat->getName().data()) { + dbgs() << "Map StringRef: '" << Name << "' @ " << (void *)Name.data() + << "\n"; + dbgs() << "Pat String: '" << Pat->getName() << "' @ " + << (void *)Pat->getName().data() << "\n"; + PrintFatalError("StringRef stored in the PatternMap is not referencing " + "the same string as its Pattern!"); } } }; @@ -745,7 +761,7 @@ // Look by pattern name, e.g. // (G_FNEG $x, $y):$root if (auto It = MatchPats.find(RootName); It != MatchPats.end()) { - MatchRoot = MatchPats[RootName].get(); + MatchRoot = It->second.get(); return true; } @@ -769,8 +785,8 @@ bool CombineRuleBuilder::buildOperandsTable() { // Walk each instruction pattern - for (auto &P : MatchPats) { - auto *IP = dyn_cast(P.getValue().get()); + for (auto &[_, P] : MatchPats) { + auto *IP = dyn_cast(P.get()); if (!IP) continue; for (const auto &Operand : IP->operands()) { @@ -790,8 +806,8 @@ } } - for (auto &P : ApplyPats) { - auto *IP = dyn_cast(P.getValue().get()); + for (auto &[_, P] : ApplyPats) { + auto *IP = dyn_cast(P.get()); if (!IP) continue; for (const auto &Operand : IP->operands()) { @@ -913,7 +929,7 @@ PrintWarning(RuleDef.getLoc(), "'match' C++ code does not seem to return!"); } - MatchPats[Name] = std::move(CXXPat); + MatchPats[CXXPat->getName()] = std::move(CXXPat); continue; } @@ -940,9 +956,9 @@ } const StringInit *Code = dyn_cast(Apply.getArg(0)); - const auto PatName = makeAnonPatName("apply"); - ApplyPats[PatName] = - std::make_unique(*Code, PatName, /*IsApply*/ true); + auto Pat = std::make_unique(*Code, makeAnonPatName("apply"), + /*IsApply*/ true); + ApplyPats[Pat->getName()] = std::move(Pat); return true; } @@ -1003,20 +1019,19 @@ return false; // Emit remaining patterns - for (auto &Entry : MatchPats) { - Pattern *CurPat = Entry.getValue().get(); - if (SeenPats.contains(CurPat)) + for (auto &[_, Pat] : MatchPats) { + if (SeenPats.contains(Pat.get())) continue; - switch (CurPat->getKind()) { + switch (Pat->getKind()) { case Pattern::K_AnyOpcode: PrintError("wip_match_opcode can not be used with instruction patterns!"); return false; case Pattern::K_Inst: - cast(CurPat)->reportUnreachable(RuleDef.getLoc()); + cast(Pat.get())->reportUnreachable(RuleDef.getLoc()); return false; case Pattern::K_CXX: { - addCXXPredicate(IM, CE, *cast(CurPat)); + addCXXPredicate(IM, CE, *cast(Pat.get())); continue; } default: @@ -1043,20 +1058,20 @@ IM.addPredicate(CGI); // Emit remaining patterns. - for (auto &Entry : MatchPats) { - Pattern *CurPat = Entry.getValue().get(); - if (CurPat == &AOP) + for (auto &[_, Pat] : MatchPats) { + if (Pat.get() == &AOP) continue; - switch (CurPat->getKind()) { + switch (Pat->getKind()) { case Pattern::K_AnyOpcode: PrintError("wip_match_opcode can only be present once!"); return false; case Pattern::K_Inst: - cast(CurPat)->reportUnreachable(RuleDef.getLoc()); + cast(Pat.get())->reportUnreachable( + RuleDef.getLoc()); return false; case Pattern::K_CXX: { - addCXXPredicate(IM, CE, *cast(CurPat)); + addCXXPredicate(IM, CE, *cast(Pat.get())); break; } default: @@ -1072,14 +1087,13 @@ } bool CombineRuleBuilder::emitApplyPatterns(CodeExpansions &CE, RuleMatcher &M) { - for (auto &Entry : ApplyPats) { - Pattern *CurPat = Entry.getValue().get(); - switch (CurPat->getKind()) { + for (auto &[_, Pat] : ApplyPats) { + switch (Pat->getKind()) { case Pattern::K_AnyOpcode: case Pattern::K_Inst: llvm_unreachable("Unsupported pattern kind in output pattern!"); case Pattern::K_CXX: { - CXXPattern *CXXPat = cast(CurPat); + CXXPattern *CXXPat = cast(Pat.get()); const auto &ExpandedCode = CXXPat->expandCode(CE, RuleDef.getLoc()); M.addAction( ExpandedCode.getEnumNameWithPrefix(CXXApplyPrefix));