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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab accepted this revision.
ab added a comment.
This revision is now accepted and ready to land.

Assuming Kristof and Diana agree, LGTM with Diana's comments (and a couple more of my own) addressed.



================
Comment at: test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-muladd.mir:1
+# RUN: llc -O0 -mtriple=aarch64-linux-gnu -run-pass=instruction-select -verify-machineinstrs -global-isel %s -o - | FileCheck %s
+
----------------
Rename the tests to match the others, "test/CodeGen/AArch64/select-*.mir" ?


================
Comment at: test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-muladd.mir:10
+---
+#  =>  (SMADDLrrr:i64 GPR32:i32:$Rn, GPR32:i32:$Rm, GPR64:i64:$Ra)
+
----------------
Stale comment?


================
Comment at: test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-sext.mir:6
+
+  define void @sext_s16_s32_gpr() { ret void }
+  define void @sext_s32_s64_gpr() { ret void }
----------------
We already have select-int-ext.mir.  Can you keep the tests together?  (there are enough to keep them in separate files now, though)


================
Comment at: test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-sext.mir:12
+---
+# CHECK-LABEL: name: sext_s16_s32_gpr
+name:            sext_s16_s32_gpr
----------------
-> sext_s32_s16_gpr to match the operand order?  (and more importantly, the existing tests!)


================
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
 
----------------
dsanders wrote:
> 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.
Can you fix the new tests as well?


================
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 {}
----------------
dsanders wrote:
> 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.
Can you undo it?


https://reviews.llvm.org/D30539