[globalisel] Re-factor ISel matchers into a hierarchy. NFC

[dsanders]

Summary

This should make it possible to easily add everything needed to import all
the existing SelectionDAG rules. It should also serve the likely
kinds of GlobalISel rules (some of which are not currently representable
in SelectionDAG) once we've nailed down the tablegen definition for that.

The hierarchy is as follows:

MatcherRule - A matching rule. Currently used to emit C++ ISel code but will
|             also be used to emit test cases and tablegen definitions in the
|             near future.
|- Instruction(s) - Represents the instruction to be matched.
   |- Instruction Predicate(s) - Test the opcode, arithmetic flags, etc. of an
   |                             instruction.
   \- Operand(s) - Represents a particular operand of the instruction. In the
      |            future, there may be subclasses to test the same predicates
      |            on multiple operands (including for variadic instructions).
      \ Operand Predicate(s) - Test the type, register bank, etc. of an operand.
                               This is where the ComplexPattern equivalent
                               will be represented. It's also
                               nested-instruction matching will live as a
                               predicate that follows the DefUse chain to the
                               Def and tests a MatcherRule from that position.

Support for multiple instruction matchers in a rule has been retained from
the existing code but has been adjusted to assert when it is used.
Previously it would silently drop all but the first instruction matcher.

The tablegen-erated file is not functionally changed but has more
parentheses and no longer attempts to format the if-statements since
keeping track of the indentation is tricky in the presence of the matcher
hierarchy. It would be nice to have CMakes tablegen() run the output
through clang-format (when available) so we don't have to complicate
TableGen with pretty-printing.

It's also worth mentioning that this hierarchy will also be able to emit
TableGen definitions and test cases in the near future. This is the reason
for favouring explicit emit*() calls rather than the << operator.

[dsanders]

Fix a silly mistake. RegisterOperandPredicate is testing the LLT for the register not the register itself.

[kristof.beyls]

utils/TableGen/GlobalISelEmitter.cpp

283

s/that// ?

285–287

Comments like this help to understand the code at least a little bit to someone like me who hasn't looked at tablegen much before. Please keep them :)

326–329

It stood out to me that the method with the same name on class OperandMatcher has the following, slightly different implementation:

template <class Kind, class... Args>
Kind &addPredicate(Args... args) {
  Predicates.emplace_back(make_unique<Kind>(std::forward<Args...>(args)...));
  return *static_cast<Kind *>(Predicates.back().get());
}

Any reason it has to be different?
Any reasonable way possible to not have to repeat this implementation, but be able to share a single implementation between both classes?

341–346

I think this needs a comment to explain what a MatcherRule is/does.
I think the comments on the classes above are really helpful to be able to understand the code and intent quickly.

342–353

To make the generated code more efficient, we'd probably want to play heuristics here so that the most discriminating predicate gets checked first?
Although I guess that's an optimization that can wait for later, until this design has proven to work well?

349–352

My guess it'd make sense for Matchers and Actions to either both be private or both be public?
My personal preference would be both private, but I'm not sure if the coding standard has anything to say on this.

[ab]

This seems reasonable to me. One quick question: how do you envision nested patterns/instructions? Would one of the parent's OperandMatchers have some sort of "instruction" predicate?

[dsanders]

One quick question: how do you envision nested patterns/instructions? Would one of the parent's OperandMatchers have some sort of "instruction" predicate?

That's right. I'm expecting to have an OperandPredicateMatcher subclass that follows the DefUse chain to the def. It will then use an InstructionMatcher to match the nested instruction.

I'm still thinking through a couple details of the generated code that this case will need but it doesn't affect the representation of within tablegen. One issue is that it's possible for side-effects/memory/etc. between the matched instructions to prevent the match. I don't want to implement that by walking the intervening instructions since that's likely to be slow and might involve traversing the CFG in the long-run. The other one is what to do if the nested instruction has multiple uses since there's no single good answer for all situations. It depends on the target and whether the priority is performance/size/energy.

utils/TableGen/GlobalISelEmitter.cpp

326–329

Any reason it has to be different?

Well spotted. They're not supposed to be different.

Any reasonable way possible to not have to repeat this implementation, but be able to share a single implementation between both classes?

I think I can have InstructionMatcher and OperandMatcher subclass a template that defines the predicates and predicate-related methods. I'll take a look

341–346

Ok. I'll also rename it to RuleMatcher to make it consistent with InstructionMatcher/OperandMatcher

342–353

I agree. I think we should do that once we're successfully importing most of the SelectionDAG rules since there's likely to be a fair bit of iteration as we gain support for the more complicated ones.

Another optimization that will be important when we get to that will be hoisting common predicates out of the rules to reduce the amount that need to be checked. The main one will be the opcode check since implementing that as a switch that picks between different rule sets will dramatically cut down on the number of rules to check.

349–352

I think they should both be private but I wanted to stick to the matchers for this patch.

I'm also not sure we need support for multiple actions, since they aren't as easy to compose as the matchers are. My initial thoughts on the actions were to have a mutate operation (to just change the opcode), a simple replacement operation, an arbitrary C++ operation, and maybe a couple other common special cases.

[dsanders]

• Moved predicate handling from OperandMatcher and InstructionMatcher to a common base class template WithPredicates. I don't seem to be able to factor out the emitCxxPredicateExpr() loop though. I should be able to use the same technique as for addPredicate() but it doesn't work. I'm looking into it.
• Add missing const's
• Fix comment typos
• Rename MatcherRule to RuleMatcher for consistency

[ab]

In D28942#653240, @dsanders wrote:

I should be able to use the same technique as for addPredicate() but it doesn't work. I'm looking into it.

Have you figured that out? Other than that, LGTM;

utils/TableGen/GlobalISelEmitter.cpp

124

The name is cryptic ...but I don't have a better idea ¯\_(ツ)_/¯

PredicateList? PredicateVector? PredicateHolder?

[dsanders]

Move appropriate emitCxxPredicatesExpr() methods into WithPredicates.

The '#if 0' block is just to provide an example of the version that doesn't
work. It's not intended for commit.

[kristof.beyls]

utils/TableGen/GlobalISelEmitter.cpp

124

I'm not entirely sure, but would PredicateMatcher be more in line with the terminology of the other classes used here?
And maybe the class hierarchy becomes a bit more simple to understand if the inheritance went roughly like follows:

class PredicateMatcher; /* rename of current WithPredicates. */
class OperandPredicateMatcher : PredicateMatcher<OperandPredicateMatcher>;
class InstructionPredicateMatcher : PredicateMatcher<InstructionPredicateMatcher>;
class OperandMatcher;
class InstructionMatcher;

rather than the current

class WithPredicates; /* rename of current WithPredicates. */
class OperandPredicateMatcher : PredicateMatcher<OperandPredicateMatcher>;
class InstructionPredicateMatcher : PredicateMatcher<InstructionPredicateMatcher>;
class OperandMatcher : WithPredicates<OperandPredicateMatcher>;
class InstructionMatcher : WithPredicates<InstructionPredicateMatcher>;

I'm afraid I haven't had much thought about this, but the first seems a bit more intuitive/easier to understand to me, assuming it's possible at all to define the class hierarchy like that.

Anyway, as I think we're still in the exploring phase for how the tablegen-generator will work, I don't think it's worthwhile to obsess too much on the last polishing touches of the OO-design of this part of the tablegen-generator right now, if it would be delaying further exploration of the tablegen-generator.

[dsanders]

In D28942#655757, @ab wrote:

In D28942#653240, @dsanders wrote:

I should be able to use the same technique as for addPredicate() but it doesn't work. I'm looking into it.

Have you figured that out?

Not entirely. I've updated the patch to one that has a workaround that's ok-ish but I don't understand why the version of the code in the #if 0 block doesn't work. I've have thought that addPredicate() and emitCxxPredicates() would either both work or both fail.

utils/TableGen/GlobalISelEmitter.cpp

124

I struggled with ideas for the name too. The only reason I went with 'WithPredicates' is because it made the subclass declaration read nicely:

class OperandMatcher : public WithPredicates<OperandPredicateMatcher>

which defines an matcher with predicates.

I'll have another think on naming.

[dsanders]

utils/TableGen/GlobalISelEmitter.cpp

124

I'm not entirely sure, but would PredicateMatcher be more in line with the terminology of the other classes used here?

It would need to be PredicatesMatcher since it provides functions to add and emit code for multiple predicates.

And maybe the class hierarchy becomes a bit more simple to understand if the inheritance went roughly like follows:

class PredicateMatcher; /* rename of current WithPredicates. */
class OperandPredicateMatcher : PredicateMatcher<OperandPredicateMatcher>;
class InstructionPredicateMatcher : PredicateMatcher<InstructionPredicateMatcher>;
class OperandMatcher;
class InstructionMatcher;

In this hierarchy, each predicate would contain a list of predicates.

[kristof.beyls]

utils/TableGen/GlobalISelEmitter.cpp

124

Ah yes. PredicatesMatcher as a name is more clear to me than "WithPredicates". The hierarchy you have in your most recent patch also seems to make sense to me. Thanks for explaining!

[dsanders]

utils/TableGen/GlobalISelEmitter.cpp

124

Thinking about it a bit more this morning. It's easy to mis-read PredicatesMatcher as PredicateMatcher which then makes the hierarchy look wrong. I'll go with PredicateListMatcher to make that a bit clearer.

[ab]

utils/TableGen/GlobalISelEmitter.cpp

124

SGTM; maybe rename emitCxxPredicatesExpr to emitCxxPredicateListExpr, for the same reason?

BTW, this naming scheme has an interesting quirk: how should we call top-level Predicates (non-instruction, as in, the actual Predicate class in Target.td). Locally, I went with the terrible but consistent PredicatePredicateMatcher. Any ideas?

163

I attempted to merge these into one in r293214; was that what you had in mind? Looks like std::forward<Args...> should have been std::forward<Args> ?

[dsanders]

utils/TableGen/GlobalISelEmitter.cpp

124

maybe rename emitCxxPredicatesExpr to emitCxxPredicateListExpr, for the same reason?

Good point. I'll do that in the morning.

BTW, this naming scheme has an interesting quirk: how should we call top-level Predicates (non-
instruction, as in, the actual Predicate class in Target.td). Locally, I went with the terrible but consistent
PredicatePredicateMatcher. Any ideas?

I'd suggest RulePredicateMatcher since they control whether we should attempt to apply a RuleMatcher.

PatternPredicateMatcher would also make sense if we go by the existing SelectionDAG classes.

163

Yep, that's what I was aiming for.

Given that I had it wrong there, it will also be wrong in addPredicate(). Maybe I got away with it there because no caller has >=2 arguments.

utils/TableGen/GlobalISelEmitter.cpp

 }
 
 static bool isTrivialOperatorNode(const TreePatternNode *N) {
   return !N->isLeaf() && !N->hasAnyPredicate() && !N->getTransformFn();
 }
 
 //===- Matchers -----------------------------------------------------------===//
 
-struct Matcher {
-  virtual ~Matcher() {}
-  virtual void emit(raw_ostream &OS) const = 0;
-};
-
-raw_ostream &operator<<(raw_ostream &S, const Matcher &M) {
-  M.emit(S);
-  return S;
-}
-
 struct MatchAction {
   virtual ~MatchAction() {}
   virtual void emit(raw_ostream &OS) const = 0;
 };
 
 raw_ostream &operator<<(raw_ostream &S, const MatchAction &A) {
   A.emit(S);
   return S;
 }
 
-struct MatchOpcode : public Matcher {
-  MatchOpcode(const CodeGenInstruction *I) : I(I) {}
-  const CodeGenInstruction *I;
+template <class PredicateTy>
+class WithPredicates {
+private:
+  typedef std::vector<std::unique_ptr<PredicateTy>> PredicateVec;
+  PredicateVec Predicates;
 
-  virtual void emit(raw_ostream &OS) const {
-    OS << "I.getOpcode() == " << I->Namespace << "::" << I->TheDef->getName();
+public:
+  /// Construct a new operand predicate and add it to the matcher.
+  template <class Kind, class... Args>
+  Kind &addPredicate(Args&&... args) {
+    Predicates.emplace_back(make_unique<Kind>(std::forward<Args...>(args)...));
+    return *static_cast<Kind *>(Predicates.back().get());
+  }
+
+  typename PredicateVec::const_iterator predicates_begin() const { return Predicates.begin(); }
+  typename PredicateVec::const_iterator predicates_end() const { return Predicates.end(); }
+  iterator_range<typename PredicateVec::const_iterator> predicates() const {
+    return make_range(predicates_begin(), predicates_end());
+  }
+
+#if 0
+  /// Emit a C++ expression that tests whether all the predicates are met.
+  template <class... Args>
+  void emitCxxPredicatesExpr(raw_ostream &OS, Args&&... args) const {
+    if (Predicates.empty()) {
+      OS << "true";
+      return;
+    }
+
+    StringRef Separator = "";
+    for (const auto &Predicate : predicates()) {
+      OS << Separator << "(";
+      Predicate->emitCxxPredicateExpr(OS, std::forward<Args...>(args)...);
+      OS << ")";
+      Separator = " && ";
+    }
+  }
+#else
+  /// Emit a C++ expression that tests whether all the predicates are met.
+  template <class Arg1>
+  void emitCxxPredicatesExpr(raw_ostream &OS, Arg1&& arg1) const {
+    if (Predicates.empty()) {
+      OS << "true";
+      return;
     }
+
+    StringRef Separator = "";
+    for (const auto &Predicate : predicates()) {
+      OS << Separator << "(";
+      Predicate->emitCxxPredicateExpr(OS, std::forward<Arg1>(arg1));
+      OS << ")";
+      Separator = " && ";
+    }
+  }
+
+  template <class Arg1, class Arg2>
+  void emitCxxPredicatesExpr(raw_ostream &OS, Arg1&& arg1, Arg2&& arg2) const {
+    if (Predicates.empty()) {
+      OS << "true";
+      return;
+    }
+
+    StringRef Separator = "";
+    for (const auto &Predicate : predicates()) {
+      OS << Separator << "(";
+      Predicate->emitCxxPredicateExpr(OS, std::forward<Arg1>(arg1),
+                                      std::forward<Arg2>(arg2));
+      OS << ")";
+      Separator = " && ";
+    }
+  }
+#endif
 };
 
-struct MatchRegOpType : public Matcher {
-  MatchRegOpType(unsigned OpIdx, std::string Ty)
-      : OpIdx(OpIdx), Ty(Ty) {}
-  unsigned OpIdx;
+/// Generates code to check a predicate of an operand.
+///
+/// Typical predicates include:
+/// * Operand is a particular register.
+/// * Operand is assigned a particular register bank.
+/// * Operand is an MBB.
+class OperandPredicateMatcher {
+public:
+  virtual ~OperandPredicateMatcher() {}
+
+  /// Emit a C++ expression that checks the predicate for the OpIdx operand of
+  /// the instruction given in InsnVarName.
+  virtual void emitCxxPredicateExpr(raw_ostream &OS,
+                                    const StringRef InsnVarName,
+                                    unsigned OpIdx) const = 0;
+};
+
+/// Generates code to check that an operand is a particular LLT.
+class LLTOperandMatcher : public OperandPredicateMatcher {
+protected:
   std::string Ty;
 
-  virtual void emit(raw_ostream &OS) const {
-    OS << "MRI.getType(I.getOperand(" << OpIdx << ").getReg()) == (" << Ty
-       << ")";
+public:
+  LLTOperandMatcher(std::string Ty) : Ty(Ty) {}
+
+  void emitCxxPredicateExpr(raw_ostream &OS, const StringRef InsnVarName,
+                            unsigned OpIdx) const override {
+    OS << "MRI.getType(" << InsnVarName << ".getOperand(" << OpIdx
+       << ").getReg()) == (" << Ty << ")";
   }
 };
 
-struct MatchRegOpBank : public Matcher {
-  MatchRegOpBank(unsigned OpIdx, const CodeGenRegisterClass &RC)
-      : OpIdx(OpIdx), RC(RC) {}
-  unsigned OpIdx;
+/// Generates code to check that an operand is in a particular register bank.
+class RegisterBankOperandMatcher : public OperandPredicateMatcher {
+protected:
   const CodeGenRegisterClass &RC;
 
-  virtual void emit(raw_ostream &OS) const {
+public:
+  RegisterBankOperandMatcher(const CodeGenRegisterClass &RC) : RC(RC) {}
+
+  void emitCxxPredicateExpr(raw_ostream &OS, const StringRef InsnVarName,
+                            unsigned OpIdx) const override {
     OS << "(&RBI.getRegBankFromRegClass(" << RC.getQualifiedName()
-       << "RegClass) == RBI.getRegBank(I.getOperand(" << OpIdx
-       << ").getReg(), MRI, TRI))";
+       << "RegClass) == RBI.getRegBank(" << InsnVarName << ".getOperand("
+       << OpIdx << ").getReg(), MRI, TRI))";
   }
 };
 
-struct MatchMBBOp : public Matcher {
-  MatchMBBOp(unsigned OpIdx) : OpIdx(OpIdx) {}
+/// Generates code to check that an operand is a basic block.
+class MBBOperandMatcher : public OperandPredicateMatcher {
+public:
+  void emitCxxPredicateExpr(raw_ostream &OS, const StringRef InsnVarName,
+                            unsigned OpIdx) const override {
+    OS << InsnVarName << ".getOperand(" << OpIdx << ").isMBB()";
+  }
+};
+
+/// Generates code to check that a set of predicates match for a particular
+/// operand.
+class OperandMatcher : public WithPredicates<OperandPredicateMatcher> {
+protected:
   unsigned OpIdx;
 
-  virtual void emit(raw_ostream &OS) const {
-    OS << "I.getOperand(" << OpIdx << ").isMBB()";
+public:
+  OperandMatcher(unsigned OpIdx) : OpIdx(OpIdx) {}
+
+  /// Emit a C++ expression that tests whether the instruction named in
+  /// InsnVarName matches all the predicate and all the operands.
+  void emitCxxPredicateExpr(raw_ostream &OS, const StringRef InsnVarName) const {
+    OS << "(";
+    emitCxxPredicatesExpr(OS, InsnVarName, OpIdx);
+    OS << ")";
+  }
+};
+
+/// Generates code to check a predicate on an instruction.
+///
+/// Typical predicates include:
+/// * The opcode of the instruction is a particular value.
+/// * The nsw/nuw flag is/isn't set.
+class InstructionPredicateMatcher {
+public:
+  virtual ~InstructionPredicateMatcher() {}
+
+  /// Emit a C++ expression that tests whether the instruction named in
+  /// InsnVarName matches the predicate.
+  virtual void emitCxxPredicateExpr(raw_ostream &OS,
+                                    const StringRef InsnVarName) const = 0;
+};
+
+/// Generates code to check the opcode of an instruction.
+class InstructionOpcodeMatcher : public InstructionPredicateMatcher {
+protected:
+  const CodeGenInstruction *I;
+
+public:
+  InstructionOpcodeMatcher(const CodeGenInstruction *I) : I(I) {}
+
+  void emitCxxPredicateExpr(raw_ostream &OS,
+                            const StringRef InsnVarName) const override {
+    OS << InsnVarName << ".getOpcode() == " << I->Namespace
+       << "::" << I->TheDef->getName();
+  }
+};
+
+/// Generates code to check that a set of predicates and operands match for a
+/// particular instruction.
+///
+/// Typical predicates include:
+/// * Has a specific opcode.
+/// * Has an nsw/nuw flag or doesn't.
+class InstructionMatcher : public WithPredicates<InstructionPredicateMatcher> {
+protected:
+  std::vector<OperandMatcher> Operands;
+
+public:
+  /// Add an operand to the matcher.
+  OperandMatcher &addOperand(unsigned OpIdx) {
+    Operands.emplace_back(OpIdx);
+    return Operands.back();
+  }
+
+  /// Emit a C++ expression that tests whether the instruction named in
+  /// InsnVarName matches all the predicates and all the operands.
+  void emitCxxPredicateExpr(raw_ostream &OS, const StringRef InsnVarName) const {
+    emitCxxPredicatesExpr(OS, InsnVarName);
+    for (const auto &Operand : Operands) {
+      OS << " && (";
+      Operand.emitCxxPredicateExpr(OS, InsnVarName);
+      OS << ")";
+    }
   }
 };
 
 struct MutateOpcode : public MatchAction {
   MutateOpcode(const CodeGenInstruction *I) : I(I) {}
   const CodeGenInstruction *I;
 
   virtual void emit(raw_ostream &OS) const {
     OS << "I.setDesc(TII.get(" << I->Namespace << "::" << I->TheDef->getName()
        << "));";
   }
 };
 
-class MatcherEmitter {
+/// Generates code to check that a match rule matches.
+///
+/// This currently supports a single match position but could be extended to
+/// support multiple positions to support div/rem fusion or load-multiple
+/// instructions.
+class RuleMatcher {
   const PatternToMatch &P;
 
+  std::vector<std::unique_ptr<InstructionMatcher>> Matchers;
+
 public:
-  std::vector<std::unique_ptr<Matcher>> Matchers;
   std::vector<std::unique_ptr<MatchAction>> Actions;
 
-  MatcherEmitter(const PatternToMatch &P) : P(P) {}
+  RuleMatcher(const PatternToMatch &P) : P(P) {}
+
+  InstructionMatcher &addInstructionMatcher() {
+    Matchers.emplace_back(new InstructionMatcher());
+    return *Matchers.back();
+  }
 
   void emit(raw_ostream &OS) {
     if (Matchers.empty())
       llvm_unreachable("Unexpected empty matcher!");
 
     OS << "  // Src: " << *P.getSrcPattern() << "\n"
        << "  // Dst: " << *P.getDstPattern() << "\n";
 
-    OS << "  if ((" << *Matchers.front() << ")";
-    for (auto &MA : makeArrayRef(Matchers).drop_front())
-      OS << " &&\n      (" << *MA << ")";
+    // The representation supports rules that require multiple roots such as:
+    //    %ptr(p0) = ...
+    //    %elt0(s32) = G_LOAD %ptr
+    //    %1(p0) = G_ADD %ptr, 4
+    //    %elt1(s32) = G_LOAD p0 %1
+    // which could be usefully folded into:
+    //    %ptr(p0) = ...
+    //    %elt0(s32), %elt1(s32) = TGT_LOAD_PAIR %ptr
+    // on some targets but we don't need to make use of that yet.
+    assert(Matchers.size() == 1 && "Cannot handle multi-root matchers yet");
+    OS << "  if (";
+    Matchers.front()->emitCxxPredicateExpr(OS, "I");
     OS << ") {\n";
 
     for (auto &MA : Actions)
       OS << "    " << *MA << "\n";
 
     OS << "    constrainSelectedInstRegOperands(I, TII, TRI, RBI);\n";
     OS << "    return true;\n";
     OS << "  }\n";
GlobalISelEmitter::GlobalISelEmitter(RecordKeeper &RK)
     : RK(RK), CGP(RK), Target(CGP.getTargetInfo()) {}
 
 //===- Emitter ------------------------------------------------------------===//
 
 Optional<GlobalISelEmitter::SkipReason>
 GlobalISelEmitter::runOnPattern(const PatternToMatch &P, raw_ostream &OS) {
 
   // Keep track of the matchers and actions to emit.
-  MatcherEmitter M(P);
+  RuleMatcher M(P);
 
   // First, analyze the whole pattern.
   // If the entire pattern has a predicate (e.g., target features), ignore it.
   if (!P.getPredicates()->getValues().empty())
     return SkipReason{"Pattern has a predicate"};
 
   // Physreg imp-defs require additional logic.  Ignore the pattern.
   if (!P.getDstRegs().empty())
GlobalISelEmitter::runOnPattern(const PatternToMatch &P, raw_ostream &OS) {
   auto &DstI = Target.getInstruction(DstOp);
 
   auto SrcGIOrNull = findNodeEquiv(Src->getOperator());
   if (!SrcGIOrNull)
     return SkipReason{"Pattern operator lacks an equivalent Instruction"};
   auto &SrcGI = *SrcGIOrNull;
 
   // The operators look good: match the opcode and mutate it to the new one.
-  M.Matchers.emplace_back(new MatchOpcode(&SrcGI));
+  InstructionMatcher &InsnMatcher = M.addInstructionMatcher();
+  InsnMatcher.addPredicate<InstructionOpcodeMatcher>(&SrcGI);
   M.Actions.emplace_back(new MutateOpcode(&DstI));
 
   // Next, analyze the children, only accepting patterns that don't require
   // any change to operands.
   if (Src->getNumChildren() != Dst->getNumChildren())
     return SkipReason{"Src/dst patterns have a different # of children"};
 
   unsigned OpIdx = 0;
 
   // Start with the defined operands (i.e., the results of the root operator).
   if (DstI.Operands.NumDefs != Src->getExtTypes().size())
     return SkipReason{"Src pattern results and dst MI defs are different"};
 
   for (const EEVT::TypeSet &Ty : Src->getExtTypes()) {
     Record *DstIOpRec = DstI.Operands[OpIdx].Rec;
     if (!DstIOpRec->isSubClassOf("RegisterClass"))
       return SkipReason{"Dst MI def isn't a register class"};
 
     auto OpTyOrNone = MVTToLLT(Ty.getConcrete());
     if (!OpTyOrNone)
       return SkipReason{"Dst operand has an unsupported type"};
 
-    M.Matchers.emplace_back(new MatchRegOpType(OpIdx, *OpTyOrNone));
-    M.Matchers.emplace_back(
-        new MatchRegOpBank(OpIdx, Target.getRegisterClass(DstIOpRec)));
+    OperandMatcher &OM = InsnMatcher.addOperand(OpIdx);
+    OM.addPredicate<LLTOperandMatcher>(*OpTyOrNone);
+    OM.addPredicate<RegisterBankOperandMatcher>(
+        Target.getRegisterClass(DstIOpRec));
     ++OpIdx;
   }
 
   // Finally match the used operands (i.e., the children of the root operator).
   for (unsigned i = 0, e = Src->getNumChildren(); i != e; ++i) {
     auto *SrcChild = Src->getChild(i);
     auto *DstChild = Dst->getChild(i);
 
     // Patterns can reorder operands.  Ignore those for now.
     if (SrcChild->getName() != DstChild->getName())
       return SkipReason{"Src/dst pattern children not in same order"};
 
     // The only non-leaf child we accept is 'bb': it's an operator because
     // BasicBlockSDNode isn't inline, but in MI it's just another operand.
     if (!SrcChild->isLeaf()) {
       if (DstChild->isLeaf() ||
           SrcChild->getOperator() != DstChild->getOperator())
         return SkipReason{"Src/dst pattern child operator mismatch"};
 
       if (SrcChild->getOperator()->isSubClassOf("SDNode")) {
         auto &ChildSDNI = CGP.getSDNodeInfo(SrcChild->getOperator());
         if (ChildSDNI.getSDClassName() == "BasicBlockSDNode") {
-          M.Matchers.emplace_back(new MatchMBBOp(OpIdx++));
+          InsnMatcher.addOperand(OpIdx++).addPredicate<MBBOperandMatcher>();
           continue;
         }
       }
       return SkipReason{"Src pattern child isn't a leaf node"};
     }
 
     if (SrcChild->getLeafValue() != DstChild->getLeafValue())
       return SkipReason{"Src/dst pattern child leaf mismatch"};
 
     // Otherwise, we're looking for a bog-standard RegisterClass operand.
     if (SrcChild->hasAnyPredicate())
       return SkipReason{"Src pattern child has predicate"};
     auto *ChildRec = cast<DefInit>(SrcChild->getLeafValue())->getDef();
     if (!ChildRec->isSubClassOf("RegisterClass"))
       return SkipReason{"Src pattern child isn't a RegisterClass"};
 
     ArrayRef<EEVT::TypeSet> ChildTypes = SrcChild->getExtTypes();
     if (ChildTypes.size() != 1)
       return SkipReason{"Src pattern child has multiple results"};
 
     auto OpTyOrNone = MVTToLLT(ChildTypes.front().getConcrete());
     if (!OpTyOrNone)
       return SkipReason{"Src operand has an unsupported type"};
 
-    M.Matchers.emplace_back(new MatchRegOpType(OpIdx, *OpTyOrNone));
-    M.Matchers.emplace_back(
-        new MatchRegOpBank(OpIdx, Target.getRegisterClass(ChildRec)));
+    OperandMatcher &OM = InsnMatcher.addOperand(OpIdx);
+    OM.addPredicate<LLTOperandMatcher>(*OpTyOrNone);
+    OM.addPredicate<RegisterBankOperandMatcher>(
+        Target.getRegisterClass(ChildRec));
     ++OpIdx;
   }
 
   // We're done with this pattern!  Emit the processed result.
   M.emit(OS);
   ++NumPatternEmitted;
   return None;
 }