[globalisel] Make the MatchAction hierarchy consistent with the matchers. NFC.

[kristof.beyls]

utils/TableGen/GlobalISelEmitter.cpp

294 ↗

(On Diff #86398)

For more consistency, maybe this class name should end in "Action", i.e. "MutateOpcodeAction"?
All of the matcher sub-class names end in "Matcher".

[dsanders]

MutateOpcode -> MutateOpcodeAction

[dsanders]

utils/TableGen/GlobalISelEmitter.cpp

294 ↗

(On Diff #86398)

I agree. I've updated the patch

[kristof.beyls]

utils/TableGen/GlobalISelEmitter.cpp

330 ↗

(On Diff #86403)

The corresponding function addPredicate(Args&&... args) has the same implementation, except for this return statement:

return *static_cast<Kind *>(Predicates.back().get());

Any reason why it needs to be different?

[dsanders]

Fixed a silly inconsistency between addPredicate() and addAction(). The return
value isn't used at the moment so I hadn't noticed I'd used MatchAction instead
of Kind.

[dsanders]

utils/TableGen/GlobalISelEmitter.cpp

330 ↗

(On Diff #86403)

Thanks for noticing. There's no reason behind it, it's just a silly mistake on my part. I've updated the patch

[kristof.beyls]

utils/TableGen/GlobalISelEmitter.cpp

330 ↗

(On Diff #86403)

Thanks, the patch LGTM now.

llvm/trunk/utils/TableGen/GlobalISelEmitter.cpp

 }
 
 static bool isTrivialOperatorNode(const TreePatternNode *N) {
   return !N->isLeaf() && !N->hasAnyPredicate() && !N->getTransformFn();
 }
 
 //===- Matchers -----------------------------------------------------------===//
 
-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;
-}
-
 template <class PredicateTy> class PredicateListMatcher {
 private:
   typedef std::vector<std::unique_ptr<PredicateTy>> PredicateVec;
   PredicateVec Predicates;
 
 public:
   /// Construct a new operand predicate and add it to the matcher.
   template <class Kind, class... Args>
void emitCxxPredicateExpr(raw_ostream &OS, const StringRef InsnVarName) const {
     for (const auto &Operand : Operands) {
       OS << " && (";
       Operand.emitCxxPredicateExpr(OS, InsnVarName);
       OS << ")";
     }
   }
 };
 
-struct MutateOpcode : public MatchAction {
-  MutateOpcode(const CodeGenInstruction *I) : I(I) {}
+//===- Actions ------------------------------------------------------------===//
+
+class MatchAction {
+public:
+  virtual ~MatchAction() {}
+  virtual void emitCxxActionStmts(raw_ostream &OS) const = 0;
+};
+
+class MutateOpcodeAction : public MatchAction {
+private:
   const CodeGenInstruction *I;
 
-  virtual void emit(raw_ostream &OS) const {
+public:
+  MutateOpcodeAction(const CodeGenInstruction *I) : I(I) {}
+
+  virtual void emitCxxActionStmts(raw_ostream &OS) const {
     OS << "I.setDesc(TII.get(" << I->Namespace << "::" << I->TheDef->getName()
        << "));";
   }
 };
 
 /// 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;
+  std::vector<std::unique_ptr<MatchAction>> Actions;
 
 public:
-  std::vector<std::unique_ptr<MatchAction>> Actions;
 
   RuleMatcher(const PatternToMatch &P) : P(P) {}
 
   InstructionMatcher &addInstructionMatcher() {
     Matchers.emplace_back(new InstructionMatcher());
     return *Matchers.back();
   }
 
+  template <class Kind, class... Args>
+  Kind &addAction(Args&&... args) {
+    Actions.emplace_back(llvm::make_unique<Kind>(std::forward<Args>(args)...));
+    return *static_cast<Kind *>(Actions.back().get());
+  }
+
   void emit(raw_ostream &OS) {
     if (Matchers.empty())
       llvm_unreachable("Unexpected empty matcher!");
 
     OS << "  // Src: " << *P.getSrcPattern() << "\n"
        << "  // Dst: " << *P.getDstPattern() << "\n";
 
     // 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";
+    for (const auto &MA : Actions) {
+      OS << "    ";
+      MA->emitCxxActionStmts(OS);
+      OS << "\n";
+    }
 
     OS << "    constrainSelectedInstRegOperands(I, TII, TRI, RBI);\n";
     OS << "    return true;\n";
     OS << "  }\n";
   }
 };
 
 //===- GlobalISelEmitter class --------------------------------------------===//
GlobalISelEmitter::runOnPattern(const PatternToMatch &P, raw_ostream &OS) {
   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.
   InstructionMatcher &InsnMatcher = M.addInstructionMatcher();
   InsnMatcher.addPredicate<InstructionOpcodeMatcher>(&SrcGI);
-  M.Actions.emplace_back(new MutateOpcode(&DstI));
+  M.addAction<MutateOpcodeAction>(&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;