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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders 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]->getOperand(1).isCImm() &&
+             "Expected CImm operand");
----------------
rovka wrote:
> It would be really easy to also handle .isImm() operands here. They can't appear if you always use MachineIRBuilder, but IIRC the MIRParser sometimes introduces them, so it wouldn't hurt to support them to avoid surprising people when they write tests.
Ok.

> IIRC the MIRParser sometimes introduces them, so it
> wouldn't hurt to support them to avoid surprising people
> when they write tests.

You get an isImm() operand when you write '123' instead of 'i32 123'.


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-imm.mir:29
+
+    %0(s32) = G_CONSTANT i32 1234
+    %w0 = COPY %0(s32)
----------------
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.


https://reviews.llvm.org/D35833