[PATCH] D35833: [globalisel][tablegen] Add support for importing 'imm' operands.

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:353
+      assert(OutMIs[NewInsnID] && "Attempted to add to undefined instruction");
+      assert(State.MIs[OldInsnID]->getOpcode() && "Expected G_CONSTANT");
+      if (State.MIs[OldInsnID]->getOperand(1).isCImm()) {
----------------
You forgot the == G_CONSTANT


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-imm.mir:29
+
+    %0(s32) = G_CONSTANT i32 1234
+    %w0 = COPY %0(s32)
----------------
dsanders wrote:
> rovka wrote:
> > Nit: Maybe test a negative constant instead, to make sure we do the right extension?
> Done. This revealed that a small mistake was preventing my code from being executed. The GIM_CheckNumOperands was checking for 1 operand instead of 2 (because Src had no children). The test only passed because the C++ was catching it. I've fixed this and killed the C++ that was handling it. I've also fixed a bug this revealed where ADD8ri had higher priority than INC8r on X86
> 
> This also turned up a small inconsistency between the old C++ and tablegen. The C++ uses ZExt but TableGen uses SExt. I haven't decided how to fix this one yet. For AArch64, I should presumably switch to ZExt but this is the wrong thing to do on some targets such as Mips where all register-width integers are signed.
Is there a reason why the old code would ZExt instead of SExt? SExt seems more natural in this context, but I admittedly haven't thought too much about it. 


https://reviews.llvm.org/D35833