[GlobalISelMatchTable] Don't hoist C++ predicates over operand recorders #159329
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The pattern optimizations in GlobalISelMatchTable.cpp can extract common predicates out of pattern alternatives by putting the pattern alternatives into a GroupMatcher and moving common predicates into the GroupMatcher's predicate list. This patch adds checks to avoid hoisting a common predicate before matchers that record named operands that the predicate uses, which would lead to segfaults when the imported patterns are matched. See the added test for a concrete example inspired by the AMDGPU backend. This fixes a bug encountered in #143881.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@llvm/pr-subscribers-tablegen @llvm/pr-subscribers-llvm-globalisel Author: Fabian Ritter (ritter-x2a) ChangesThe pattern optimizations in GlobalISelMatchTable.cpp can extract common See the added test for a concrete example inspired by the AMDGPU backend. This fixes a bug encountered in #143881. Full diff: https://github.com/llvm/llvm-project/pull/159329.diff 3 Files Affected:
diff --git a/llvm/test/TableGen/GlobalISelEmitter/MatchTableOptimizerInvalidHoisting.td b/llvm/test/TableGen/GlobalISelEmitter/MatchTableOptimizerInvalidHoisting.td
new file mode 100644
index 0000000000000..9ad7e052fbb14
--- /dev/null
+++ b/llvm/test/TableGen/GlobalISelEmitter/MatchTableOptimizerInvalidHoisting.td
@@ -0,0 +1,43 @@
+// RUN: llvm-tblgen %s -gen-global-isel -optimize-match-table=true -I %p/../../../include -I %p/../Common | FileCheck %s
+
+include "llvm/Target/Target.td"
+include "GlobalISelEmitterCommon.td"
+
+def InstThreeOperands : I<(outs GPR32:$dst), (ins GPR32:$src0, GPR32:$src1, GPR32:$src2), []>;
+
+class ThreeOpFrag<SDPatternOperator op1, SDPatternOperator op2> : PatFrag<
+ (ops node:$x, node:$y, node:$z),
+ (op2 (op1 node:$x, node:$y), node:$z),
+ [{
+ return Operands[0] && Operands[1] && Operands[2];
+ }]> {
+ let PredicateCodeUsesOperands = 1;
+ let GISelPredicateCode = [{
+ return Operands[0] && Operands[1] && Operands[2];
+ }];
+}
+
+def ptradd_commutative : PatFrags<(ops node:$src0, node:$src1),
+ [(ptradd node:$src0, node:$src1), (ptradd node:$src1, node:$src0)]>;
+
+// ptradd_commutative has two PatFrags, therefore there are two ways how the
+// below pattern could match. Both require checking the C++ predicate, but that
+// check cannot be hoisted because it relies on recorded operands, which differ
+// between the PatFrags. This is inspired by a similar construct in the AMDGPU
+// backend.
+
+// CHECK: GIM_Try, /*On fail goto*//*Label 1*/
+// CHECK: GIM_RecordNamedOperand
+// CHECK: GIM_RecordNamedOperand
+// CHECK: GIM_RecordNamedOperand
+// CHECK: GIM_CheckCxxInsnPredicate
+// CHECK: // Label 1
+// CHECK: GIM_Try, /*On fail goto*//*Label 2*/
+// CHECK: GIM_RecordNamedOperand
+// CHECK: GIM_RecordNamedOperand
+// CHECK: GIM_RecordNamedOperand
+// CHECK: GIM_CheckCxxInsnPredicate
+// CHECK: // Label 2
+def : Pat <(i32 (ThreeOpFrag<shl, ptradd_commutative> GPR32:$src0, GPR32:$src1, GPR32:$src2)),
+ (InstThreeOperands GPR32:$src0, GPR32:$src1, GPR32:$src2)> ;
+
diff --git a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
index 8c8d5d77ebd73..7ba9f3ed2e6c4 100644
--- a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
+++ b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
@@ -155,6 +155,10 @@ static std::string getEncodedEmitStr(StringRef NamedValue, unsigned NumBytes) {
llvm_unreachable("Unsupported number of bytes!");
}
+template <class Range> static bool matchersRecordOperand(Range &&R) {
+ return any_of(R, [](const auto &I) { return I->recordsOperand(); });
+}
+
//===- Global Data --------------------------------------------------------===//
std::set<LLTCodeGen> llvm::gi::KnownTypes;
@@ -457,6 +461,10 @@ Matcher::~Matcher() {}
//===- GroupMatcher -------------------------------------------------------===//
+bool GroupMatcher::recordsOperand() const {
+ return matchersRecordOperand(Conditions) || matchersRecordOperand(Matchers);
+}
+
bool GroupMatcher::candidateConditionMatches(
const PredicateMatcher &Predicate) const {
@@ -486,12 +494,25 @@ std::unique_ptr<PredicateMatcher> GroupMatcher::popFirstCondition() {
return P;
}
+/// Check if the Condition, which is a predicate of M, cannot be hoisted outside
+/// of (i.e., checked before) M.
+static bool cannotHoistCondition(const PredicateMatcher &Condition,
+ const Matcher &M) {
+ // The condition can't be hoisted if it is a C++ predicate that refers to
+ // operands and the operands are registered within the matcher.
+
+ return Condition.dependsOnOperands() && M.recordsOperand();
+}
+
bool GroupMatcher::addMatcher(Matcher &Candidate) {
if (!Candidate.hasFirstCondition())
return false;
+ // Only add candidates that have a matching first condition that can be
+ // hoisted into the GroupMatcher.
const PredicateMatcher &Predicate = Candidate.getFirstCondition();
- if (!candidateConditionMatches(Predicate))
+ if (!candidateConditionMatches(Predicate) ||
+ cannotHoistCondition(Predicate, Candidate))
return false;
Matchers.push_back(&Candidate);
@@ -509,10 +530,17 @@ void GroupMatcher::finalize() {
for (const auto &Rule : Matchers)
if (!Rule->hasFirstCondition())
return;
+ // Hoist the first condition if it is identical in all matchers in the group
+ // and it can be hoisted in every matcher.
const auto &FirstCondition = FirstRule.getFirstCondition();
- for (unsigned I = 1, E = Matchers.size(); I < E; ++I)
- if (!Matchers[I]->getFirstCondition().isIdentical(FirstCondition))
+ if (cannotHoistCondition(FirstCondition, FirstRule))
+ return;
+ for (unsigned I = 1, E = Matchers.size(); I < E; ++I) {
+ const auto &OtherFirstCondition = Matchers[I]->getFirstCondition();
+ if (!OtherFirstCondition.isIdentical(FirstCondition) ||
+ cannotHoistCondition(OtherFirstCondition, *Matchers[I]))
return;
+ }
Conditions.push_back(FirstRule.popFirstCondition());
for (unsigned I = 1, E = Matchers.size(); I < E; ++I)
@@ -569,6 +597,12 @@ void GroupMatcher::optimize() {
//===- SwitchMatcher ------------------------------------------------------===//
+bool SwitchMatcher::recordsOperand() const {
+ assert(!isa_and_present<RecordNamedOperandMatcher>(Condition.get()) &&
+ "Switch conditions should not record named operands");
+ return matchersRecordOperand(Matchers);
+}
+
bool SwitchMatcher::isSupportedPredicateType(const PredicateMatcher &P) {
return isa<InstructionOpcodeMatcher>(P) || isa<LLTOperandMatcher>(P);
}
@@ -709,6 +743,10 @@ StringRef RuleMatcher::getOpcode() const {
return Matchers.front()->getOpcode();
}
+bool RuleMatcher::recordsOperand() const {
+ return matchersRecordOperand(Matchers);
+}
+
LLTCodeGen RuleMatcher::getFirstConditionAsRootType() {
InstructionMatcher &InsnMatcher = *Matchers.front();
if (!InsnMatcher.predicates_empty())
@@ -1378,6 +1416,10 @@ TempTypeIdx OperandMatcher::getTempTypeIdx(RuleMatcher &Rule) {
return TTIdx;
}
+bool OperandMatcher::recordsOperand() const {
+ return matchersRecordOperand(Predicates);
+}
+
void OperandMatcher::emitPredicateOpcodes(MatchTable &Table,
RuleMatcher &Rule) {
if (!Optimized) {
@@ -1759,6 +1801,10 @@ OperandMatcher &InstructionMatcher::addPhysRegInput(const Record *Reg,
return *OM;
}
+bool InstructionMatcher::recordsOperand() const {
+ return matchersRecordOperand(Predicates) || matchersRecordOperand(operands());
+}
+
void InstructionMatcher::emitPredicateOpcodes(MatchTable &Table,
RuleMatcher &Rule) {
if (canAddNumOperandsCheck()) {
diff --git a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h
index 13f29e10beba2..a310fc82d081b 100644
--- a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h
+++ b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h
@@ -313,6 +313,10 @@ class Matcher {
virtual bool hasFirstCondition() const = 0;
virtual const PredicateMatcher &getFirstCondition() const = 0;
virtual std::unique_ptr<PredicateMatcher> popFirstCondition() = 0;
+
+ /// Check recursively if the matcher records named operands for use in C++
+ /// predicates.
+ virtual bool recordsOperand() const = 0;
};
class GroupMatcher final : public Matcher {
@@ -374,6 +378,8 @@ class GroupMatcher final : public Matcher {
}
bool hasFirstCondition() const override { return !Conditions.empty(); }
+ bool recordsOperand() const override;
+
private:
/// See if a candidate matcher could be added to this group solely by
/// analyzing its first condition.
@@ -439,6 +445,8 @@ class SwitchMatcher : public Matcher {
bool hasFirstCondition() const override { return false; }
+ bool recordsOperand() const override;
+
private:
/// See if the predicate type has a Switch-implementation for it.
static bool isSupportedPredicateType(const PredicateMatcher &Predicate);
@@ -669,6 +677,8 @@ class RuleMatcher : public Matcher {
void optimize() override;
void emit(MatchTable &Table) override;
+ bool recordsOperand() const override;
+
/// Compare the priority of this object and B.
///
/// Returns true if this object is more important than B.
@@ -858,6 +868,8 @@ class PredicateMatcher {
return Kind == IPM_GenericPredicate;
}
+ bool recordsOperand() const { return Kind == OPM_RecordNamedOperand; }
+
virtual bool isIdentical(const PredicateMatcher &B) const {
return B.getKind() == getKind() && InsnVarID == B.InsnVarID &&
OpIdx == B.OpIdx;
@@ -1322,6 +1334,8 @@ class OperandMatcher : public PredicateListMatcher<OperandPredicateMatcher> {
/// already been assigned, simply returns it.
TempTypeIdx getTempTypeIdx(RuleMatcher &Rule);
+ bool recordsOperand() const;
+
std::string getOperandExpr(unsigned InsnVarID) const;
InstructionMatcher &getInstructionMatcher() const { return Insn; }
@@ -1840,6 +1854,8 @@ class InstructionMatcher final : public PredicateListMatcher<PredicateMatcher> {
void optimize();
+ bool recordsOperand() const;
+
/// Emit MatchTable opcodes that test whether the instruction named in
/// InsnVarName matches all the predicates and all the operands.
void emitPredicateOpcodes(MatchTable &Table, RuleMatcher &Rule);
|
arsenm
approved these changes
Sep 17, 2025
Merge activity
|
ritter-x2a
deleted the
users/ritter-x2a/09-17-_globaliselmatchtable_don_t_hoist_c_predicates_over_operand_recorders
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The pattern optimizations in GlobalISelMatchTable.cpp can extract common
predicates out of pattern alternatives by putting the pattern alternatives into
a GroupMatcher and moving common predicates into the GroupMatcher's predicate
list. This patch adds checks to avoid hoisting a common predicate before
matchers that record named operands that the predicate uses, which would lead
to segfaults when the imported patterns are matched.
See the added test for a concrete example inspired by the AMDGPU backend.
This fixes a bug encountered in #143881.