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

[Diana Picus via Phabricator via llvm-commits]

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

Seems fine.



================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:352
+      int64_t OldInsnID = MatchTable[CurrentIdx++];
+      assert(OutMIs[NewInsnID] && "Attempted to add to undefined instruction");
+      assert(State.MIs[OldInsnID]->getOperand(1).isCImm() &&
----------------
You could also assert that the OldInsn is a G_CONSTANT.


================
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");
----------------
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.


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-imm.mir:29
+
+    %0(s32) = G_CONSTANT i32 1234
+    %w0 = COPY %0(s32)
----------------
Nit: Maybe test a negative constant instead, to make sure we do the right extension?


https://reviews.llvm.org/D35833