Skip to content

Commit

Permalink
[TableGen][GlobalISel] Guarantee stable iteration order for stop-afte…
Browse files Browse the repository at this point in the history 
…r-parse

Builds on top of 6de2735 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
  • Loading branch information
@Pierre-vh
Pierre-vh committed Jul 24, 2023
1 parent 92a11eb commit d7c6d05
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 43 deletions.
4 changes: 2 additions & 2 deletions llvm/test/TableGen/GlobalISelCombinerMatchTableEmitter/pattern-parsing.td
View file
Expand Up @@ -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
Expand Down
96 changes: 55 additions & 41 deletions llvm/utils/TableGen/GlobalISelCombinerMatchTableEmitter.cpp
View file
Expand Up @@ -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;
};

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -614,20 +618,20 @@ 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";
return;
}

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";
Expand Down Expand Up @@ -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!");
}
}
};
Expand Down Expand Up @@ -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;
}

Expand All @@ -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()) {
Expand All @@ -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()) {
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand All @@ -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));
Expand Down

0 comments on commit d7c6d05

Please sign in to comment.