[GlobalISel] Fix AArch64 ICMP instruction selection

[kristof.beyls]

Summary

CSINC increments the result by one when the condition is false. The existing code seems to assume it increases the result by one if the condition is true.
The simple fix is to inverse the predicate during ICMP instruction selection.

This dramatically increases the execution pass rate in the test-suite run with -mllvm -global-isel=true -mllvm -global-isel-abort=1 from 13.7% to 47.2%!
Not too surprising giving we were always lowering ICMP incorrectly so far...

[qcolombet]

Hi Kristof,

LGTM.
Nitpicks below.

Thanks,
-Quentin

lib/Target/AArch64/AArch64InstructionSelector.cpp

1079 ↗

(On Diff #82727)

Could you add a comment why we do this?
Exactly what you said in the summary of the patch would be good.

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

if (CmpTy == LLT::scalar(32)) {
       ZReg = AArch64::WZR;
     } else if (CmpTy == LLT::scalar(64) || CmpTy.isPointer()) {
       CmpOpc = AArch64::SUBSXrr;
       ZReg = AArch64::XZR;
     } else {
       return false;
     }
 
-    const AArch64CC::CondCode CC = changeICMPPredToAArch64CC(
-        (CmpInst::Predicate)I.getOperand(1).getPredicate());
+    // CSINC increments the result by one when the condition code is false.
+    // Therefore, we have to invert the predicate to get an increment by 1 when
+    // the predicate is true.
+    const AArch64CC::CondCode invCC =
+        changeICMPPredToAArch64CC(CmpInst::getInversePredicate(
+            (CmpInst::Predicate)I.getOperand(1).getPredicate()));
 
     MachineInstr &CmpMI = *BuildMI(MBB, I, I.getDebugLoc(), TII.get(CmpOpc))
                                .addDef(ZReg)
                                .addUse(I.getOperand(2).getReg())
                                .addUse(I.getOperand(3).getReg());
 
     MachineInstr &CSetMI =
         *BuildMI(MBB, I, I.getDebugLoc(), TII.get(AArch64::CSINCWr))
              .addDef(I.getOperand(0).getReg())
              .addUse(AArch64::WZR)
              .addUse(AArch64::WZR)
-             .addImm(CC);
+             .addImm(invCC);
 
     constrainSelectedInstRegOperands(CmpMI, TII, TRI, RBI);
     constrainSelectedInstRegOperands(CSetMI, TII, TRI, RBI);
 
     I.eraseFromParent();
     return true;
   }
 

llvm/trunk/test/CodeGen/AArch64/GlobalISel/arm64-instructionselect.mir

registers:
   - { id: 1, class: gpr }
   - { id: 2, class: gpr }
   - { id: 3, class: gpr }
   - { id: 4, class: gpr }
   - { id: 5, class: gpr }
 
 # CHECK:  body:
 # CHECK:    %wzr = SUBSWrr %0, %0, implicit-def %nzcv
-# CHECK:    %1 = CSINCWr %wzr, %wzr, 0, implicit %nzcv
+# CHECK:    %1 = CSINCWr %wzr, %wzr, 1, implicit %nzcv
 
 # CHECK:    %xzr = SUBSXrr %2, %2, implicit-def %nzcv
-# CHECK:    %3 = CSINCWr %wzr, %wzr, 2, implicit %nzcv
+# CHECK:    %3 = CSINCWr %wzr, %wzr, 3, implicit %nzcv
 
 # CHECK:    %xzr = SUBSXrr %4, %4, implicit-def %nzcv
-# CHECK:    %5 = CSINCWr %wzr, %wzr, 1, implicit %nzcv
+# CHECK:    %5 = CSINCWr %wzr, %wzr, 0, implicit %nzcv
 
 body:             |
   bb.0:
     liveins: %w0, %x0
 
     %0(s32) = COPY %w0
     %1(s1) = G_ICMP intpred(eq), %0, %0