[llvm] d7c6d05 - [TableGen][GlobalISel] Guarantee stable iteration order for stop-after-parse
via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 24 00:10:55 PDT 2023
Author: pvanhout
Date: 2023-07-24T09:10:50+02:00
New Revision: d7c6d057efa1ebe6fdd878f12f75bc52b8cd933e
URL: https://github.com/llvm/llvm-project/commit/d7c6d057efa1ebe6fdd878f12f75bc52b8cd933e
DIFF: https://github.com/llvm/llvm-project/commit/d7c6d057efa1ebe6fdd878f12f75bc52b8cd933e.diff
LOG: [TableGen][GlobalISel] Guarantee stable iteration order for stop-after-parse
Builds on top of 6de2735c2428 to fix remaining issues with iteration order in the MatchTable Combiner backend.
See D155789 as well.
Reviewed By: MaskRay
Differential Revision: https://reviews.llvm.org/D155821
Added:
Modified:
llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-parsing.td
llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp
Removed:
################################################################################
diff --git a/llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-parsing.td b/llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-parsing.td
index c0bacf9cea36f6..51786762ee7d88 100644
--- a/llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-parsing.td
+++ b/llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-parsing.td
@@ -81,16 +81,16 @@ def InstTest0 : GICombineRule<
// 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:[<def>x, a])
// CHECK-NEXT: <root>d:(InstructionPattern inst:MOV operands:[<def>a, b])
+// CHECK-NEXT: __anon_pat_match_3_0:(InstructionPattern inst:ZEXT operands:[<def>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
index c1186fb324dcd7..5ed3d4bb98c16a 100644
--- a/llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp
@@ -233,6 +233,10 @@ class Pattern {
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 @@ struct OperandTableEntry {
/// - 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<std::unique_ptr<Pattern>>;
+ using PatternMap = MapVector<StringRef, std::unique_ptr<Pattern>>;
CombineRuleBuilder(const CodeGenTarget &CGT,
SubtargetFeatureInfoMap &SubtargetFeatures,
@@ -547,8 +551,8 @@ class CombineRuleBuilder {
/// 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 @@ void CombineRuleBuilder::print(raw_ostream &OS) const {
OS << " )\n";
}
- const auto DumpPats = [&](StringRef Name, const PatternStringMap &Pats) {
+ const auto DumpPats = [&](StringRef Name, const PatternMap &Pats) {
OS << " (" << Name << " ";
if (Pats.empty()) {
OS << "<empty>)\n";
@@ -622,12 +626,12 @@ void CombineRuleBuilder::print(raw_ostream &OS) const {
}
OS << "\n";
- for (const auto &P : Pats) {
+ for (const auto &[Name, Pat] : Pats) {
OS << " ";
- if (P.getValue().get() == MatchRoot)
+ if (Pat.get() == MatchRoot)
OS << "<root>";
- 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::print(raw_ostream &OS) const {
}
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 @@ bool CombineRuleBuilder::findRoots() {
// 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::findRoots() {
bool CombineRuleBuilder::buildOperandsTable() {
// Walk each instruction pattern
- for (auto &P : MatchPats) {
- auto *IP = dyn_cast<InstructionPattern>(P.getValue().get());
+ for (auto &[_, P] : MatchPats) {
+ auto *IP = dyn_cast<InstructionPattern>(P.get());
if (!IP)
continue;
for (const auto &Operand : IP->operands()) {
@@ -790,8 +806,8 @@ bool CombineRuleBuilder::buildOperandsTable() {
}
}
- for (auto &P : ApplyPats) {
- auto *IP = dyn_cast<InstructionPattern>(P.getValue().get());
+ for (auto &[_, P] : ApplyPats) {
+ auto *IP = dyn_cast<InstructionPattern>(P.get());
if (!IP)
continue;
for (const auto &Operand : IP->operands()) {
@@ -913,7 +929,7 @@ bool CombineRuleBuilder::parseMatch(DagInit &Match) {
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 @@ bool CombineRuleBuilder::parseApply(DagInit &Apply) {
}
const StringInit *Code = dyn_cast<StringInit>(Apply.getArg(0));
- const auto PatName = makeAnonPatName("apply");
- ApplyPats[PatName] =
- std::make_unique<CXXPattern>(*Code, PatName, /*IsApply*/ true);
+ auto Pat = std::make_unique<CXXPattern>(*Code, makeAnonPatName("apply"),
+ /*IsApply*/ true);
+ ApplyPats[Pat->getName()] = std::move(Pat);
return true;
}
@@ -1003,20 +1019,19 @@ bool CombineRuleBuilder::emitMatchPattern(CodeExpansions &CE,
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<InstructionPattern>(CurPat)->reportUnreachable(RuleDef.getLoc());
+ cast<InstructionPattern>(Pat.get())->reportUnreachable(RuleDef.getLoc());
return false;
case Pattern::K_CXX: {
- addCXXPredicate(IM, CE, *cast<CXXPattern>(CurPat));
+ addCXXPredicate(IM, CE, *cast<CXXPattern>(Pat.get()));
continue;
}
default:
@@ -1043,20 +1058,20 @@ bool CombineRuleBuilder::emitMatchPattern(CodeExpansions &CE,
IM.addPredicate<InstructionOpcodeMatcher>(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<InstructionPattern>(CurPat)->reportUnreachable(RuleDef.getLoc());
+ cast<InstructionPattern>(Pat.get())->reportUnreachable(
+ RuleDef.getLoc());
return false;
case Pattern::K_CXX: {
- addCXXPredicate(IM, CE, *cast<CXXPattern>(CurPat));
+ addCXXPredicate(IM, CE, *cast<CXXPattern>(Pat.get()));
break;
}
default:
@@ -1072,14 +1087,13 @@ bool CombineRuleBuilder::emitMatchPattern(CodeExpansions &CE,
}
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<CXXPattern>(CurPat);
+ CXXPattern *CXXPat = cast<CXXPattern>(Pat.get());
const auto &ExpandedCode = CXXPat->expandCode(CE, RuleDef.getLoc());
M.addAction<CustomCXXAction>(
ExpandedCode.getEnumNameWithPrefix(CXXApplyPrefix));
More information about the llvm-commits
mailing list