[globalisel][tablegen] Fix PR35375 by sign-extending the table value to match getConstantVRegVal()

[dsanders]

Summary

From the bug report:

The problem is that it fails when trying to compare -65536 (or 4294901760) to 0xFFFF,0000. This is because the
constant in the instruction is sign extended to 64 bits (0xFFFF,FFFF,FFFF,0000) and then compared to the non
extended 64 bit version expected by TableGen.

In contrast, the DAGISelEmitter generates special code for AND immediates (OPC_CheckAndImm), which does not
sign extend.

This patch doesn't introduce the special case for AND (and OR) immediates since the majority of it is related to handling known bits that have no effect on the result and GlobalISel doesn't detect known-bits at this time. Instead this patch just ensures that the immediate is extended consistently on both sides of the check.

Thanks to Diana Picus for the detailed bug report.

[rovka]

I don't know if this fixes every edge case that we currently have with and/or immediates in DAGISel, but it works for the PKHBT and doesn't seem to break anything else, so in the interest of simplicity it's probably a good route to take. You should add a summary when you commit though.

[dsanders]

I think GIM_CheckLiteralInt might have the same issue but I don't think there's any existing rules that would detect the problem. As far as I know X86 is the only user and only uses 0 and 1. We should probably change it to match GIM_CheckConstantInt if only for consistency. Do you agree?

[rovka]

In D40532#938284, @dsanders wrote:

I think GIM_CheckLiteralInt might have the same issue but I don't think there's any existing rules that would detect the problem. As far as I know X86 is the only user and only uses 0 and 1. We should probably change it to match GIM_CheckConstantInt if only for consistency. Do you agree?

That sounds like a good idea, and I think we can test with the MOV32r_1 pattern, which uses a -1. I think for something like this, it should select a MOV32r_1:

--- |
  define i32 @const_i32_0() #0 {
    ret i32 0
  }

  attributes #0 = { optsize "target-features"="-64bit-mode"}
...
---
name:            const_i32_0
legalized:       true
regBankSelected: true
registers:
  - { id: 0, class: gpr }
body:             |
  bb.1 (%ir-block.0):
    %0(s32) = G_CONSTANT i32 -1
    %eax = COPY %0(s32)
    RET 0, implicit %eax
...

I'll let @igorb correct me if I'm wrong...

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h

case GIM_CheckConstantInt: {
       int64_t InsnID = MatchTable[CurrentIdx++];
       int64_t OpIdx = MatchTable[CurrentIdx++];
       int64_t Value = MatchTable[CurrentIdx++];
       DEBUG_WITH_TYPE(TgtInstructionSelector::getName(),
                       dbgs() << CurrentIdx << ": GIM_CheckConstantInt(MIs["
                              << InsnID << "]->getOperand(" << OpIdx
                              << "), Value=" << Value << ")\n");
       assert(State.MIs[InsnID] != nullptr && "Used insn before defined");
+
+      // isOperandImmEqual() will sign-extend to 64-bits, so should we.
+      LLT Ty = MRI.getType(State.MIs[InsnID]->getOperand(OpIdx).getReg());
+      Value = SignExtend64(Value, Ty.getSizeInBits());
+
       if (!isOperandImmEqual(State.MIs[InsnID]->getOperand(OpIdx), Value,
                              MRI)) {
         if (handleReject() == RejectAndGiveUp)
           return false;
       }
       break;
     }
 

test/CodeGen/ARM/GlobalISel/pr35375.ll

+; RUN: llc -O0 -mtriple armv7-- -stop-before=expand-isel-pseudos < %s
+; RUN: llc -O0 -mtriple armv7-- -stop-before=expand-isel-pseudos -global-isel < %s
+
+; CHECK: PKHBT
+
+define arm_aapcscc i32 @pkh(i32 %x, i32 %y) {
+  %andx = and i32 %x, 65535
+  %shl = shl i32 %y, 1
+  %andy = and i32 %shl, 4294901760 ; same as -65536
+  %or = or i32 %andx, %andy
+  ret i32 %or
+}