[AArch64][GlobalISel] Recognise some CCMPri

[dmgreen]

Summary

This is a simple addition to emitConditionalComparison, to match CCMP with immediates using getIConstantVRegValWithLookThrough, letting it select the CCMPri variants of the instructions.

[paquette]

LGTM, thanks!

[dmgreen]

Cheers

[fhahn]

Just a heads-up, we are seeing miscompiles that look to be caused by this patch. I'll try to share in isolated reproducer.

[dmgreen]

OK - feel free to revert. (I have no decent testing for GlobalISel yet).

Is it worth setting up a bot to police it?

[fhahn]

In D131073#3719397, @dmgreen wrote:

OK - feel free to revert. (I have no decent testing for GlobalISel yet).

It turns out the mis-compile can also be reproduced by just bootstrapping LLVM with -O3 -fglobal-isel on AArch64 hardware. I reverted the change for now.

Is it worth setting up a bot to police it?

Yes definitely. It doesn't look like we have any buildbots covering GlobalISel on AArch64. I'll see if we can at least add a bootstrapping bot.

[tschuett]

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

4850

This condition is different from the one above? The C->Value.ult(32) differs.

[dmgreen]

It turns out the mis-compile can also be reproduced by just bootstrapping LLVM with -O3 -fglobal-isel on AArch64 hardware. I reverted the change for now.

I've tested a bootstrap and it worked with the new code. Please let me know if anything else seems broken.

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

4850

Yeah, that was pretty daft. It was intended to clear the C if it wasn't set to anything useful. I've changed it to a opcode check instead.

[paquette]

Considering there was a miscompile with this last time, I think there's probably a missing testcase. Could you add a test which covers the part that was broken last time?

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

4850

It'd be nice to have a comment here

[dmgreen]

The recommit in https://reviews.llvm.org/rG3c6edc0b2f812503a802bdd9c5842a0a9396b8c1 has a new test case and checks on the opcode, not the const.

[paquette]

Oh, good, thank you!

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

MachineInstr *AArch64InstructionSelector::emitConditionalComparison(
     Register LHS, Register RHS, CmpInst::Predicate CC,
     AArch64CC::CondCode Predicate, AArch64CC::CondCode OutCC,
     MachineIRBuilder &MIB) const {
   // TODO: emit CMN as an optimization.
   auto &MRI = *MIB.getMRI();
   LLT OpTy = MRI.getType(LHS);
   assert(OpTy.getSizeInBits() == 32 || OpTy.getSizeInBits() == 64);
   unsigned CCmpOpc;
+  Optional<ValueAndVReg> C;
   if (CmpInst::isIntPredicate(CC)) {
+    C = getIConstantVRegValWithLookThrough(RHS, MRI);
+    if (C && C->Value.ult(32))
+      CCmpOpc = OpTy.getSizeInBits() == 32 ? AArch64::CCMPWi : AArch64::CCMPXi;
+    else
       CCmpOpc = OpTy.getSizeInBits() == 32 ? AArch64::CCMPWr : AArch64::CCMPXr;
   } else {
     switch (OpTy.getSizeInBits()) {
     case 16:
       CCmpOpc = AArch64::FCCMPHrr;
       break;
     case 32:
       CCmpOpc = AArch64::FCCMPSrr;
       break;
     case 64:
       CCmpOpc = AArch64::FCCMPDrr;
       break;
     default:
       return nullptr;
     }
   }
   AArch64CC::CondCode InvOutCC = AArch64CC::getInvertedCondCode(OutCC);
   unsigned NZCV = AArch64CC::getNZCVToSatisfyCondCode(InvOutCC);
   auto CCmp =
-      MIB.buildInstr(CCmpOpc, {}, {LHS, RHS}).addImm(NZCV).addImm(Predicate);
+      MIB.buildInstr(CCmpOpc, {}, {LHS});
+  if (C)
+    CCmp.addImm(C->Value.getZExtValue());
+  else
+    CCmp.addReg(RHS);
+  CCmp.addImm(NZCV).addImm(Predicate);
   constrainSelectedInstRegOperands(*CCmp, TII, TRI, RBI);
   return &*CCmp;
 }
 
 MachineInstr *AArch64InstructionSelector::emitConjunctionRec(
     Register Val, AArch64CC::CondCode &OutCC, bool Negate, Register CCOp,
     AArch64CC::CondCode Predicate, MachineIRBuilder &MIB) const {
   // We're at a tree leaf, produce a conditional comparison operation.

llvm/test/CodeGen/AArch64/arm64-ccmp.ll

 ; SDISEL-NEXT:    ccmp x1, #0, #0, eq
 ; SDISEL-NEXT:    mov w8, #1
 ; SDISEL-NEXT:    cinc x0, x8, eq
 ; SDISEL-NEXT:    ret
 ;
 ; GISEL-LABEL: gccbug:
 ; GISEL:       ; %bb.0:
 ; GISEL-NEXT:    mov w8, #2
-; GISEL-NEXT:    mov w9, #4
 ; GISEL-NEXT:    cmp x0, #2
-; GISEL-NEXT:    ccmp x0, x9, #4, ne
-; GISEL-NEXT:    ccmp x1, xzr, #0, eq
+; GISEL-NEXT:    ccmp x0, #4, #4, ne
+; GISEL-NEXT:    ccmp x1, #0, #0, eq
 ; GISEL-NEXT:    csinc x0, x8, xzr, eq
 ; GISEL-NEXT:    ret
   %cmp0 = icmp eq i64 %x1, 0
   %cmp1 = icmp eq i64 %x0, 2
   %cmp2 = icmp eq i64 %x0, 4
 
   %or = or i1 %cmp2, %cmp1
   %and = and i1 %or, %cmp0
 
   %sel = select i1 %and, i64 2, i64 1
   ret i64 %sel
 }
 
 define i32 @select_ororand(i32 %w0, i32 %w1, i32 %w2, i32 %w3) {
-; SDISEL-LABEL: select_ororand:
-; SDISEL:       ; %bb.0:
-; SDISEL-NEXT:    cmp w3, #4
-; SDISEL-NEXT:    ccmp w2, #2, #0, gt
-; SDISEL-NEXT:    ccmp w1, #13, #2, ge
-; SDISEL-NEXT:    ccmp w0, #0, #4, ls
-; SDISEL-NEXT:    csel w0, w3, wzr, eq
-; SDISEL-NEXT:    ret
-;
-; GISEL-LABEL: select_ororand:
-; GISEL:       ; %bb.0:
-; GISEL-NEXT:    mov w8, #13
-; GISEL-NEXT:    mov w9, #2
-; GISEL-NEXT:    cmp w3, #4
-; GISEL-NEXT:    ccmp w2, w9, #0, gt
-; GISEL-NEXT:    ccmp w1, w8, #2, ge
-; GISEL-NEXT:    ccmp w0, wzr, #4, ls
-; GISEL-NEXT:    csel w0, w3, wzr, eq
-; GISEL-NEXT:    ret
+; CHECK-LABEL: select_ororand:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    cmp w3, #4
+; CHECK-NEXT:    ccmp w2, #2, #0, gt
+; CHECK-NEXT:    ccmp w1, #13, #2, ge
+; CHECK-NEXT:    ccmp w0, #0, #4, ls
+; CHECK-NEXT:    csel w0, w3, wzr, eq
+; CHECK-NEXT:    ret
   %c0 = icmp eq i32 %w0, 0
   %c1 = icmp ugt i32 %w1, 13
   %c2 = icmp slt i32 %w2, 2
   %c4 = icmp sgt i32 %w3, 4
   %or = or i1 %c0, %c1
   %and = and i1 %c2, %c4
   %or1 = or i1 %or, %and
   %sel = select i1 %or1, i32 %w3, i32 0
   ret i32 %sel
 }
 
 define i32 @select_andor(i32 %v1, i32 %v2, i32 %v3) {
-; SDISEL-LABEL: select_andor:
-; SDISEL:       ; %bb.0:
-; SDISEL-NEXT:    cmp w1, w2
-; SDISEL-NEXT:    ccmp w0, #0, #4, lt
-; SDISEL-NEXT:    ccmp w0, w1, #0, eq
-; SDISEL-NEXT:    csel w0, w0, w1, eq
-; SDISEL-NEXT:    ret
-;
-; GISEL-LABEL: select_andor:
-; GISEL:       ; %bb.0:
-; GISEL-NEXT:    cmp w1, w2
-; GISEL-NEXT:    ccmp w0, wzr, #4, lt
-; GISEL-NEXT:    ccmp w0, w1, #0, eq
-; GISEL-NEXT:    csel w0, w0, w1, eq
-; GISEL-NEXT:    ret
+; CHECK-LABEL: select_andor:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    cmp w1, w2
+; CHECK-NEXT:    ccmp w0, #0, #4, lt
+; CHECK-NEXT:    ccmp w0, w1, #0, eq
+; CHECK-NEXT:    csel w0, w0, w1, eq
+; CHECK-NEXT:    ret
   %c0 = icmp eq i32 %v1, %v2
   %c1 = icmp sge i32 %v2, %v3
   %c2 = icmp eq i32 %v1, 0
   %or = or i1 %c2, %c1
   %and = and i1 %or, %c0
   %sel = select i1 %and, i32 %v1, i32 %v2
   ret i32 %sel
 }
; CHECK-NEXT:    ret
   %cr = and i1 %c1, %c0
   %sel = select i1 %cr, i32 %a, i32 %b
   ret i32 %sel
 }
 
 ; This testcase resembles the core problem of http://llvm.org/PR39550
 ; (an OR operation is 2 levels deep but needs to be implemented first)
 define i32 @deep_or(i32 %a0, i32 %a1, i32 %a2, i32 %a3, i32 %x, i32 %y) {
-; SDISEL-LABEL: deep_or:
-; SDISEL:       ; %bb.0:
-; SDISEL-NEXT:    cmp w2, #20
-; SDISEL-NEXT:    ccmp w2, #15, #4, ne
-; SDISEL-NEXT:    ccmp w1, #0, #4, eq
-; SDISEL-NEXT:    ccmp w0, #0, #4, ne
-; SDISEL-NEXT:    csel w0, w4, w5, ne
-; SDISEL-NEXT:    ret
-;
-; GISEL-LABEL: deep_or:
-; GISEL:       ; %bb.0:
-; GISEL-NEXT:    mov w8, #15
-; GISEL-NEXT:    cmp w2, #20
-; GISEL-NEXT:    ccmp w2, w8, #4, ne
-; GISEL-NEXT:    ccmp w1, wzr, #4, eq
-; GISEL-NEXT:    ccmp w0, wzr, #4, ne
-; GISEL-NEXT:    csel w0, w4, w5, ne
-; GISEL-NEXT:    ret
+; CHECK-LABEL: deep_or:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    cmp w2, #20
+; CHECK-NEXT:    ccmp w2, #15, #4, ne
+; CHECK-NEXT:    ccmp w1, #0, #4, eq
+; CHECK-NEXT:    ccmp w0, #0, #4, ne
+; CHECK-NEXT:    csel w0, w4, w5, ne
+; CHECK-NEXT:    ret
   %c0 = icmp ne i32 %a0, 0
   %c1 = icmp ne i32 %a1, 0
   %c2 = icmp eq i32 %a2, 15
   %c3 = icmp eq i32 %a2, 20
 
   %or = or i1 %c2, %c3
   %and0 = and i1 %or, %c1
   %and1 = and i1 %and0, %c0
   %sel = select i1 %and1, i32 %x, i32 %y
   ret i32 %sel
 }
 
 ; Variation of deep_or, we still need to implement the OR first though.
 define i32 @deep_or1(i32 %a0, i32 %a1, i32 %a2, i32 %a3, i32 %x, i32 %y) {
-; SDISEL-LABEL: deep_or1:
-; SDISEL:       ; %bb.0:
-; SDISEL-NEXT:    cmp w2, #20
-; SDISEL-NEXT:    ccmp w2, #15, #4, ne
-; SDISEL-NEXT:    ccmp w0, #0, #4, eq
-; SDISEL-NEXT:    ccmp w1, #0, #4, ne
-; SDISEL-NEXT:    csel w0, w4, w5, ne
-; SDISEL-NEXT:    ret
-;
-; GISEL-LABEL: deep_or1:
-; GISEL:       ; %bb.0:
-; GISEL-NEXT:    mov w8, #15
-; GISEL-NEXT:    cmp w2, #20
-; GISEL-NEXT:    ccmp w2, w8, #4, ne
-; GISEL-NEXT:    ccmp w0, wzr, #4, eq
-; GISEL-NEXT:    ccmp w1, wzr, #4, ne
-; GISEL-NEXT:    csel w0, w4, w5, ne
-; GISEL-NEXT:    ret
+; CHECK-LABEL: deep_or1:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    cmp w2, #20
+; CHECK-NEXT:    ccmp w2, #15, #4, ne
+; CHECK-NEXT:    ccmp w0, #0, #4, eq
+; CHECK-NEXT:    ccmp w1, #0, #4, ne
+; CHECK-NEXT:    csel w0, w4, w5, ne
+; CHECK-NEXT:    ret
   %c0 = icmp ne i32 %a0, 0
   %c1 = icmp ne i32 %a1, 0
   %c2 = icmp eq i32 %a2, 15
   %c3 = icmp eq i32 %a2, 20
 
   %or = or i1 %c2, %c3
   %and0 = and i1 %c0, %or
   %and1 = and i1 %and0, %c1
   %sel = select i1 %and1, i32 %x, i32 %y
   ret i32 %sel
 }
 
 ; Variation of deep_or, we still need to implement the OR first though.
 define i32 @deep_or2(i32 %a0, i32 %a1, i32 %a2, i32 %a3, i32 %x, i32 %y) {
-; SDISEL-LABEL: deep_or2:
-; SDISEL:       ; %bb.0:
-; SDISEL-NEXT:    cmp w2, #20
-; SDISEL-NEXT:    ccmp w2, #15, #4, ne
-; SDISEL-NEXT:    ccmp w1, #0, #4, eq
-; SDISEL-NEXT:    ccmp w0, #0, #4, ne
-; SDISEL-NEXT:    csel w0, w4, w5, ne
-; SDISEL-NEXT:    ret
-;
-; GISEL-LABEL: deep_or2:
-; GISEL:       ; %bb.0:
-; GISEL-NEXT:    mov w8, #15
-; GISEL-NEXT:    cmp w2, #20
-; GISEL-NEXT:    ccmp w2, w8, #4, ne
-; GISEL-NEXT:    ccmp w1, wzr, #4, eq
-; GISEL-NEXT:    ccmp w0, wzr, #4, ne
-; GISEL-NEXT:    csel w0, w4, w5, ne
-; GISEL-NEXT:    ret
+; CHECK-LABEL: deep_or2:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    cmp w2, #20
+; CHECK-NEXT:    ccmp w2, #15, #4, ne
+; CHECK-NEXT:    ccmp w1, #0, #4, eq
+; CHECK-NEXT:    ccmp w0, #0, #4, ne
+; CHECK-NEXT:    csel w0, w4, w5, ne
+; CHECK-NEXT:    ret
   %c0 = icmp ne i32 %a0, 0
   %c1 = icmp ne i32 %a1, 0
   %c2 = icmp eq i32 %a2, 15
   %c3 = icmp eq i32 %a2, 20
 
   %or = or i1 %c2, %c3
   %and0 = and i1 %c0, %c1
   %and1 = and i1 %and0, %or