[PATCH] D30539: [tablegen][globalisel] Add support for nested instruction matching.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:938
   case TargetOpcode::G_ZEXT:
   case TargetOpcode::G_SEXT: {
     unsigned Opcode = I.getOpcode();
----------------
rovka wrote:
> Aren't these handled by TableGen now?
Good point. I've forgotten to delete the C++ implementation


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-xor.mir:1
-# RUN: llc -O0 -mtriple=aarch64-- -run-pass=instruction-select -verify-machineinstrs -global-isel %s -o - | FileCheck %s
+# RUN: llc -O0 -mtriple=aarch64-linux-gnu -run-pass=instruction-select -verify-machineinstrs -global-isel %s -o - | FileCheck %s
 
----------------
rovka wrote:
> Why is this change necessary?
I think this appeared as a result rebasing over an upstream change but it shows up in my local copy of this patch too. I'll undo it.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:289
+  /// Only InstructionOperandMatcher needs to do anything for this method.
+  virtual void emitCxxCaptureStmtsForOperand(raw_ostream &OS, RuleMatcher &Rule,
+                                             StringRef Expr) const {}
----------------
rovka wrote:
> Any reason why this needs to be [...]ForOperand instead of just emitCxxCaptureStmts?
I don't recall a good reason for this. I think I did it to make it easier to navigate around the source.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:655
   /// TODO: When nested instruction matching is implemented, this function will
   ///       descend into the operands and capture variables.
   void emitCxxCaptureStmts(raw_ostream &OS, RuleMatcher &Rule, StringRef Expr) {
----------------
rovka wrote:
> This TODO should be obsolete now.
Thanks for noticing this. I'll remove it.


https://reviews.llvm.org/D30539