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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked 13 inline comments as done.
dsanders added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstructionSelector.cpp:938
   case TargetOpcode::G_ZEXT:
   case TargetOpcode::G_SEXT: {
     unsigned Opcode = I.getOpcode();
----------------
dsanders wrote:
> rovka wrote:
> > Aren't these handled by TableGen now?
> Good point. I've forgotten to delete the C++ implementation
It turns out that these are still doing something useful. They're needed for the sext_inreg/zext_inreg operator from SelectionDAG since this hasn't been mapped to GlobalISel yet


================
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 }
----------------
ab wrote:
> We already have select-int-ext.mir.  Can you keep the tests together?  (there are enough to keep them in separate files now, though)
I've merged these tests in with the recently-added select-int-ext.mir tests.


================
Comment at: test/CodeGen/AArch64/GlobalISel/arm64-instructionselect-sext.mir:12
+---
+# CHECK-LABEL: name: sext_s16_s32_gpr
+name:            sext_s16_s32_gpr
----------------
ab wrote:
> -> sext_s32_s16_gpr to match the operand order?  (and more importantly, the existing tests!)
Both orders make sense from different perspectives (mine was derived from LLVM-IR's sext/zext instruction) but I've renamed mine to match the ones that were recently committed.


================
Comment at: test/TableGen/GlobalISelEmitter.td:85
+// CHECK-NEXT:     MIB.add(MI0.getOperand(2)/*src3*/);
+// CHECK-NEXT:     MIB.setMemRefs(I.memoperands_begin(), I.memoperands_end());
+// CHECK-NEXT:     I.eraseFromParent();
----------------
ab wrote:
> On second thought, I'm not sure this is sufficient:  don't we need to merge the memops?
I spotted another issue with this yesterday. It's unusual but some targets may choose to lower a load to a non-load and in this case we shouldn't copy the memory operands across otherwise the MachineVerifier will object.

I'll have a quick look at merging the operands.


https://reviews.llvm.org/D30539