[PATCH] D30535: [tablegen][globalisel] Capture instructions into locals and related infrastructure for multiple instructions matches.

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

I think this looks ok overall, just a few nits.

There's a typo in the summary (I assume the first "capturing each" is superfluous):

> It works by capturing each checking the 'shape' of the match and capturing
>  each matched instruction to a local variable





================
Comment at: test/TableGen/GlobalISelEmitter.td:46
+// CHECK-NEXT:      return true;
+// CHECK-NEXT:    }
 
----------------
I would check for the "return true" as well.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:577
+
+  const OperandMatcher &getOperandChecked(const StringRef SymbolicName) const {
+    const OperandMatcher *OM = getOperand(SymbolicName);
----------------
Are you planning to add more checks here? If not, I would suggest naming this getOperand and renaming getOperand to getOperandOrNull (otherwise it's a bit unclear what it's checking without looking at the code).


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:595
 
+  /// Emit C++ statements to capture instructions into local variables.
+  void emitCxxCaptureStmts(raw_ostream &OS, RuleMatcher &Rule, StringRef Expr) {
----------------
This isn't capturing anything, it's just checking that we can capture. I would update the comment to reflect that, otherwise it looks a bit out-of-sync.
Ideally, we'd have a more expressive name for emitCxxCaptureStmts, but emitCxxCheckAndCaptureStmts is a bit long and I can't think of a better one, so just fixing the comments should do.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:900
   assert(Matchers.size() == 1 && "Cannot handle multi-root matchers yet");
-  OS << "  if (";
-  Matchers.front()->emitCxxPredicateExpr(OS, "I");
+  OS << "if ([&]() {\n";
+
----------------
Why is this a lambda? If it's just for readability, wouldn't it be easier to just generate comments along the lines of "Code for <blah> rule" ?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:903
+  // Check the tree is the right shape for the rule and gather the
+  // instructions into local variables.
+  emitCxxCaptureStmts(OS, "I");
----------------
I would move this comment to the emitCxxCaptureStmts declaration.


https://reviews.llvm.org/D30535