[llvm] [GlobalISelMatchTable] Don't hoist C++ predicates over operand recorders (PR #159329)
via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 17 04:46:01 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-globalisel
Author: Fabian Ritter (ritter-x2a)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/159329.diff
3 Files Affected:
- (added) llvm/test/TableGen/GlobalISelEmitter/MatchTableOptimizerInvalidHoisting.td (+43)
- (modified) llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp (+49-3)
- (modified) llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h (+16)
``````````diff
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);
``````````
</details>
https://github.com/llvm/llvm-project/pull/159329
More information about the llvm-commits
mailing list