[GlobalISelMatchTable] Don't hoist C++ predicates over operand recorders

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.

Author: ritter-x2a

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)> ;
+

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()) {

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);