[GlobalISel] Separate the SelectionDAG importer from the emitter. NFC: alternative using Expected<>

[ab]

Summary

This is an alternative to parts of D29709: instead of appending our new RuleMatcher to a vector, return it and the SkipReason as an Expected<RuleMatcher>.

It's a somewhat unconventional use of Error, but it's nicer than, say, std::pair because of the bool conversions.

[dsanders]

This will LGTM with the emitted separated from the importer like D29709 was.

I mentioned it on D29709 but just to mention it here. I'm not completely sure that runOnPattern() doesn't need to produce multiple rules for a single pattern at the moment but the original use case (commutativity) looks like it will be better handled in InstructionMatcher.

utils/TableGen/GlobalISelEmitter.cpp

409

Would this make more sense as ImportError()?

548–550

The main aim of D29709 was to separate the importer from the emitter so that there can be a sort inserted between them. As such, this needs to be in a separate loop that iterates over the whole rule-set.

[ab]

utils/TableGen/GlobalISelEmitter.cpp

409

Yeah, that's better. I went with failedImport to keep it a verb, let me know if you have a better idea!

548–550

Let me get rid of this and let's do the more interesting stuff in D29709; do you want me to rebase it onto this?

utils/TableGen/GlobalISelEmitter.cpp

 ///
 //===----------------------------------------------------------------------===//
 
 #include "CodeGenDAGPatterns.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/CodeGen/MachineValueType.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Error.h"
 #include "llvm/TableGen/Error.h"
 #include "llvm/TableGen/Record.h"
 #include "llvm/TableGen/TableGenBackend.h"
 #include <string>
 using namespace llvm;
 
 #define DEBUG_TYPE "gisel-emitter"
 
 STATISTIC(NumPatternTotal, "Total number of patterns");
 STATISTIC(NumPatternSkipped, "Number of patterns skipped");
 STATISTIC(NumPatternEmitted, "Number of patterns emitted");
 
 static cl::opt<bool> WarnOnSkippedPatterns(
     "warn-on-skipped-patterns",
     cl::desc("Explain why a pattern was skipped for inclusion "
              "in the GlobalISel selector"),
     cl::init(false));
 
 namespace {
-
-class GlobalISelEmitter {
-public:
-  explicit GlobalISelEmitter(RecordKeeper &RK);
-  void run(raw_ostream &OS);
-
-private:
-  const RecordKeeper &RK;
-  const CodeGenDAGPatterns CGP;
-  const CodeGenTarget &Target;
-
-  /// Keep track of the equivalence between SDNodes and Instruction.
-  /// This is defined using 'GINodeEquiv' in the target description.
-  DenseMap<Record *, const CodeGenInstruction *> NodeEquivs;
-
-  void gatherNodeEquivs();
-  const CodeGenInstruction *findNodeEquiv(Record *N);
-
-  struct SkipReason {
-    std::string Reason;
-  };
-
-  /// Analyze pattern \p P, possibly emitting matching code for it to \p OS.
-  /// Otherwise, return a reason why this pattern was skipped for emission.
-  Optional<SkipReason> runOnPattern(const PatternToMatch &P,
-                                    raw_ostream &OS);
-};
-
-} // end anonymous namespace
+class RuleMatcher;
 
 //===- Helper functions ---------------------------------------------------===//
 
 /// Convert an MVT to an equivalent LLT if possible, or the invalid LLT() for
 /// MVTs that don't map cleanly to an LLT (e.g., iPTR, *any, ...).
 static Optional<std::string> MVTToLLT(MVT::SimpleValueType SVT) {
   std::string TyStr;
   raw_string_ostream OS(TyStr);
void emit(raw_ostream &OS) {
     OS << "    constrainSelectedInstRegOperands(I, TII, TRI, RBI);\n";
     OS << "    return true;\n";
     OS << "  }\n\n";
   }
 };
 
 //===- GlobalISelEmitter class --------------------------------------------===//
 
+class GlobalISelEmitter {
+public:
+  explicit GlobalISelEmitter(RecordKeeper &RK);
+  void run(raw_ostream &OS);
+
+private:
+  const RecordKeeper &RK;
+  const CodeGenDAGPatterns CGP;
+  const CodeGenTarget &Target;
+
+  /// Keep track of the equivalence between SDNodes and Instruction.
+  /// This is defined using 'GINodeEquiv' in the target description.
+  DenseMap<Record *, const CodeGenInstruction *> NodeEquivs;
+
+  void gatherNodeEquivs();
+  const CodeGenInstruction *findNodeEquiv(Record *N);
+
+  /// Analyze pattern \p P, returning a matcher for it if possible.
+  /// Otherwise, return an Error explaining why we don't support it.
+  Expected<RuleMatcher> runOnPattern(const PatternToMatch &P);
+};
+
 void GlobalISelEmitter::gatherNodeEquivs() {
   assert(NodeEquivs.empty());
   for (Record *Equiv : RK.getAllDerivedDefinitions("GINodeEquiv"))
     NodeEquivs[Equiv->getValueAsDef("Node")] =
         &Target.getInstruction(Equiv->getValueAsDef("I"));
 }
 
 const CodeGenInstruction *GlobalISelEmitter::findNodeEquiv(Record *N) {
   return NodeEquivs.lookup(N);
 }
 
 GlobalISelEmitter::GlobalISelEmitter(RecordKeeper &RK)
     : RK(RK), CGP(RK), Target(CGP.getTargetInfo()) {}
 
 //===- Emitter ------------------------------------------------------------===//
 
-Optional<GlobalISelEmitter::SkipReason>
-GlobalISelEmitter::runOnPattern(const PatternToMatch &P, raw_ostream &OS) {
+/// Helper function to let the emitter report skip reason error messages.
+static Error skipBecause(const Twine &Reason) {
+  return make_error<StringError>(Reason, inconvertibleErrorCode());
+}
 
+Expected<RuleMatcher> GlobalISelEmitter::runOnPattern(const PatternToMatch &P) {
   // Keep track of the matchers and actions to emit.
   RuleMatcher M;
   M.addAction<DebugCommentAction>(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"};
+    return skipBecause("Pattern has a predicate");
 
   // Physreg imp-defs require additional logic.  Ignore the pattern.
   if (!P.getDstRegs().empty())
-    return SkipReason{"Pattern defines a physical register"};
+    return skipBecause("Pattern defines a physical register");
 
   // Next, analyze the pattern operators.
   TreePatternNode *Src = P.getSrcPattern();
   TreePatternNode *Dst = P.getDstPattern();
 
   // If the root of either pattern isn't a simple operator, ignore it.
   if (!isTrivialOperatorNode(Dst))
-    return SkipReason{"Dst pattern root isn't a trivial operator"};
+    return skipBecause("Dst pattern root isn't a trivial operator");
   if (!isTrivialOperatorNode(Src))
-    return SkipReason{"Src pattern root isn't a trivial operator"};
+    return skipBecause("Src pattern root isn't a trivial operator");
 
   Record *DstOp = Dst->getOperator();
   if (!DstOp->isSubClassOf("Instruction"))
-    return SkipReason{"Pattern operator isn't an instruction"};
+    return skipBecause("Pattern operator isn't an instruction");
 
   auto &DstI = Target.getInstruction(DstOp);
 
   auto SrcGIOrNull = findNodeEquiv(Src->getOperator());
   if (!SrcGIOrNull)
-    return SkipReason{"Pattern operator lacks an equivalent Instruction"};
+    return skipBecause("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.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"};
+    return skipBecause("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"};
+    return skipBecause("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"};
+      return skipBecause("Dst MI def isn't a register class");
 
     auto OpTyOrNone = MVTToLLT(Ty.getConcrete());
     if (!OpTyOrNone)
-      return SkipReason{"Dst operand has an unsupported type"};
+      return skipBecause("Dst operand has an unsupported type");
 
     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"};
+      return skipBecause("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"};
+        return skipBecause("Src/dst pattern child operator mismatch");
 
       if (SrcChild->getOperator()->isSubClassOf("SDNode")) {
         auto &ChildSDNI = CGP.getSDNodeInfo(SrcChild->getOperator());
         if (ChildSDNI.getSDClassName() == "BasicBlockSDNode") {
           InsnMatcher.addOperand(OpIdx++).addPredicate<MBBOperandMatcher>();
           continue;
         }
       }
-      return SkipReason{"Src pattern child isn't a leaf node"};
+      return skipBecause("Src pattern child isn't a leaf node");
     }
 
     if (SrcChild->getLeafValue() != DstChild->getLeafValue())
-      return SkipReason{"Src/dst pattern child leaf mismatch"};
+      return skipBecause("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"};
+      return skipBecause("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"};
+      return skipBecause("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"};
+      return skipBecause("Src pattern child has multiple results");
 
     auto OpTyOrNone = MVTToLLT(ChildTypes.front().getConcrete());
     if (!OpTyOrNone)
-      return SkipReason{"Src operand has an unsupported type"};
+      return skipBecause("Src operand has an unsupported type");
 
     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;
+  return std::move(M);
 }
 
 void GlobalISelEmitter::run(raw_ostream &OS) {
   // Track the GINodeEquiv definitions.
   gatherNodeEquivs();
 
   emitSourceFileHeader(("Global Instruction Selector for the " +
                        Target.getName() + " target").str(), OS);
   OS << "bool " << Target.getName()
      << "InstructionSelector::selectImpl"
         "(MachineInstr &I) const {\n  const MachineRegisterInfo &MRI = "
         "I.getParent()->getParent()->getRegInfo();\n\n";
 
   // Look through the SelectionDAG patterns we found, possibly emitting some.
   for (const PatternToMatch &Pat : CGP.ptms()) {
     ++NumPatternTotal;
-    if (auto SkipReason = runOnPattern(Pat, OS)) {
+    auto MatcherOrErr = runOnPattern(Pat);
+
+    if (auto Err = MatcherOrErr.takeError()) {
       if (WarnOnSkippedPatterns) {
         PrintWarning(Pat.getSrcRecord()->getLoc(),
-                     "Skipped pattern: " + SkipReason->Reason);
+                     "Skipped pattern: " + toString(std::move(Err)));
+      } else {
+        consumeError(std::move(Err));
       }
       ++NumPatternSkipped;
+      continue;
     }
+
+    MatcherOrErr->emit(OS);
+    ++NumPatternEmitted;
   }
 
   OS << "  return false;\n}\n";
 }
 
+} // end anonymous namespace
+
 //===----------------------------------------------------------------------===//
 
 namespace llvm {
 void EmitGlobalISel(RecordKeeper &RK, raw_ostream &OS) {
   GlobalISelEmitter(RK).run(OS);
 }
 } // End llvm namespace