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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


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


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:577
+
+  const OperandMatcher &getOperandChecked(const StringRef SymbolicName) const {
+    const OperandMatcher *OM = getOperand(SymbolicName);
----------------
rovka wrote:
> 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).
No, the 'Checked' is just to distinguish between the case that's allowed to fail and the one that isn't. I'll rename it.

What do you think about getOptionalOperand (returning an Optional<>) for the one that's allowed to fail? I think that might fit better with the support for OperandWithDefaultOps I'm looking into at the moment.


================
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) {
----------------
rovka wrote:
> 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.
Good point. It's part of the code to capture the locals but the name is misleading in this case. I can't think of any good alternatives either so I'll update the comments.


================
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";
+
----------------
rovka wrote:
> 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" ?
The main thing is that it allows me to declare MachineOperands in the middle of the checks. There's no default constructor so I can't declare them and assign to them later and I didn't want to add redundant MachineOperand::CreatePlaceholder() calls.. The alternative was to use additional scopes and 'if (!condition) goto ruleN;' but I thought that looked worse.


================
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");
----------------
rovka wrote:
> I would move this comment to the emitCxxCaptureStmts declaration.
Ok


https://reviews.llvm.org/D30535