[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]->getOpcode() && "Expected G_CONSTANT");
+      if (State.MIs[OldInsnID]->getOperand(1).isCImm()) {
----------------
rovka wrote:
> You forgot the == G_CONSTANT
Thanks. I guess I was seeing what I expected to see :-)


================
Comment at: test/CodeGen/AArch64/GlobalISel/select-imm.mir:29
+
+    %0(s32) = G_CONSTANT i32 1234
+    %w0 = COPY %0(s32)
----------------
rovka wrote:
> 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. 
I'm not sure at the moment. It could be that it makes no difference either way aside from targets like Mips which specifies the value of bits that don't physically exist in an implementation (values are theoretically sign-extended to infinite-width) and relies on this for 32-bit code to work correctly on a 64-bit CPU.

One observation from the patches I've been working on this week is that ImmLeaf's sign-extend the constant internally so it would make sense for the same operator without any predicates (`imm`) to do the same. Unfortunately, that argument can be used the other way if you start from the PatFrag's which start with an SDNode and frequently zero-extend it.


https://reviews.llvm.org/D35833