[X86] Check immediate before get it.

[LuoYuanke]

Summary

For CMP imm instruction, when the operand 1 is symbol address we should
check if it is immediate first. Here is the example code.
CMP64mi32 $noreg, 8, killed renamable $rcx, @d, $noreg, @a, implicit-def $eflags
However I am not able to create a small test case to expose this issue.

[lebedev.ri]

llvm-reduce should get you 99% of the way there.

[LuoYuanke]

Thank @lebedev.ri. This is triggered by our internal code, the same test case passes with llvm trunk code. BTW, do you think we need to check immediate before getting it?

[pengfei]

llvm/lib/Target/X86/X86InstrInfo.cpp

6410

But CMP64mi32 shouldn't go to here.

[craig.topper]

It would be nice to have a test, but this change seems ok.

llvm/lib/Target/X86/X86InstrInfo.cpp

6410

The CMPmi32 will be expanded to a load and CMPri32. This switches use the opcode after it's expanded.

[pengfei]

LGTM but let's wait one or two days to see opinions from others.

llvm/lib/Target/X86/X86InstrInfo.cpp

6410

Thanks Craig.
Discussed with Yuanke offline, I understand the fix here is to handle relocImm, i.e., @a in the example.
I think it makes sense to skip it here since we don't know it's value at this point.

[lebedev.ri]

It isn't immediately obvious to me how that can happen.
I don't think this should land without a test.

[pengfei]

Maybe we can use an MIR test?

[craig.topper]

I don't what Intel's original failure looked like, but here's a test that should reproduce this with -run-pass=machinelicm https://reviews.llvm.org/P8267 needs more cleanup.

I hacked the MIR just before machinelicm by sinking the CMP64mi32 and SETCCr into the loop. That makes MachineLICM want to unfold it since the load part is invariant being from a constant global.

[LuoYuanke]

Apply Craig's test case. Many thanks to Craig. :)

[LuoYuanke]

In D104037#2813726, @craig.topper wrote:

I don't what Intel's original failure looked like, but here's a test that should reproduce this with -run-pass=machinelicm https://reviews.llvm.org/P8267 needs more cleanup.

I hacked the MIR just before machinelicm by sinking the CMP64mi32 and SETCCr into the loop. That makes MachineLICM want to unfold it since the load part is invariant being from a constant global.

Hi Craig,

Awesome. Many thanks for the test case. I applied your test case to this patch.

[LuoYuanke]

Update test case to pass expensive check.

llvm/lib/Target/X86/X86InstrInfo.cpp

bool X86InstrInfo::unfoldMemoryOperand(
   default: break;
   case X86::CMP64ri32:
   case X86::CMP64ri8:
   case X86::CMP32ri:
   case X86::CMP32ri8:
   case X86::CMP16ri:
   case X86::CMP16ri8:
   case X86::CMP8ri: {
     MachineOperand &MO0 = DataMI->getOperand(0);
     MachineOperand &MO1 = DataMI->getOperand(1);
-    if (MO1.getImm() == 0) {
+    if (MO1.isImm() && MO1.getImm() == 0) {
       unsigned NewOpc;
       switch (DataMI->getOpcode()) {
       default: llvm_unreachable("Unreachable!");
       case X86::CMP64ri8:
       case X86::CMP64ri32: NewOpc = X86::TEST64rr; break;
       case X86::CMP32ri8:
       case X86::CMP32ri:   NewOpc = X86::TEST32rr; break;
       case X86::CMP16ri8: