Fix AArch64ConditionOptimizer

[weimingz]

Summary

This pass may modify the Cmp operands. However, the flag reg may be used by both the branch and CSEL.
Modifying CMP will have side effect on CSEL.

[weimingz]

For example:

%cmp = icmp sgt i32 %0, 0
%m = select i1 %cmp, i64 %v, i64 0
store i64 %m, i64* %p
br i1 %cmp, label %lor.lhs.false, label %land.lhs.true

will become

...

cmp w8, #1
csel x8, x0, xzr, gt // becomes x8 = (w8 > 1) ? x0 : 0
str x8, [x1]
b.lt .LBB0_2

[t.p.northover]

lib/Target/AArch64/AArch64ConditionOptimizer.cpp

177–189

This looks overly complicated to me. From the previous clause, we only care about AArch64::NZCV so I think we can skip the outer loop entirely and just look at that register.

Then we've got 2 conditions:

• NZCV isn't live-in to a successor (can be checked before we go looking for a CMP at all).
• NZCV isn't used before we find our CMP (can be checked outside the switch).

This has the added advantage that it'll be correct if we ever do decide to do something with the other compares, where exactly the same issues occur. The code in this particular switch case really will be issues specific to the actual CMP instructions we're seeing.

I don't think we need the special case for Bcc in the new scheme either: I is always before the first terminator, so we should never encounter a Bcc.

[weimingz]

Thanks for reviewing, Tim.

I move the LiveIn check to runOnMachineFunction and piggyback NZCV use check into the same loop as finding CMP because it goes backwards as well.

[weimingz]

fix indentions/spaces

[t.p.northover]

lib/Target/AArch64/AArch64ConditionOptimizer.cpp

357–361

Wouldn't this be better off at the top of findSuitableCompare? We wouldn't need the fairly weird logic to look through both BBs and would get the short-circuiting automatically.

[weimingz]

you're right :D

[t.p.northover]

Thanks Weiming! I think this looks good now.

Tim.

lib/Target/AArch64/AArch64ConditionOptimizer.cpp

MachineInstr *AArch64ConditionOptimizer::findSuitableCompare(
     MachineBasicBlock *MBB) {
   MachineBasicBlock::iterator I = MBB->getFirstTerminator();
   if (I == MBB->end())
     return nullptr;
 
   if (I->getOpcode() != AArch64::Bcc)
     return nullptr;
 
+  // Since we may modify cmp of this MBB, make sure NZCV does not live out.
+  for (auto SuccBB : MBB->successors())
+    if (SuccBB->isLiveIn(AArch64::NZCV))
+      return nullptr;
+
   // Now find the instruction controlling the terminator.
   for (MachineBasicBlock::iterator B = MBB->begin(); I != B;) {
     --I;
     assert(!I->isTerminator() && "Spurious terminator");
+    // Check if there is any use of NZCV between CMP and Bcc.
+    if (I->readsRegister(AArch64::NZCV))
+      return nullptr;
     switch (I->getOpcode()) {
     // cmp is an alias for subs with a dead destination register.
     case AArch64::SUBSWri:
     case AArch64::SUBSXri:
     // cmn is an alias for adds with a dead destination register.
     case AArch64::ADDSWri:
     case AArch64::ADDSXri: {
       unsigned ShiftAmt = AArch64_AM::getShiftValue(I->getOperand(3).getImm());
       if (!I->getOperand(2).isImm()) {
         DEBUG(dbgs() << "Immediate of cmp is symbolic, " << *I << '\n');
         return nullptr;
       } else if (I->getOperand(2).getImm() << ShiftAmt >= 0xfff) {
         DEBUG(dbgs() << "Immediate of cmp may be out of range, " << *I << '\n');
         return nullptr;
       } else if (!MRI->use_empty(I->getOperand(0).getReg())) {
         DEBUG(dbgs() << "Destination of cmp is not dead, " << *I << '\n');
         return nullptr;
       }
       return I;
     }
     // Prevent false positive case like:
     // cmp      w19, #0
     // cinc     w0, w19, gt
     // ...
     // fcmp     d8, #0.0
     // b.gt     .LBB0_5
     case AArch64::FCMPDri:
     case AArch64::FCMPSri:
     case AArch64::FCMPESri:
     case AArch64::FCMPEDri:
 
     case AArch64::SUBSWrr:
     case AArch64::SUBSXrr:
     case AArch64::ADDSWrr:
     case AArch64::ADDSXrr:
     case AArch64::FCMPSrr:
     case AArch64::FCMPDrr:
     case AArch64::FCMPESrr:
     case AArch64::FCMPEDrr:
for (MachineDomTreeNode *I : depth_first(DomTree)) {
     if (TII->AnalyzeBranch(*TBB, TBB_TBB, TBB_FBB, TrueCond)) {
       continue;
     }
 
     MachineInstr *HeadCmpMI = findSuitableCompare(HBB);
     if (!HeadCmpMI) {
       continue;
     }
 
     MachineInstr *TrueCmpMI = findSuitableCompare(TBB);
     if (!TrueCmpMI) {
       continue;
     }
 
     AArch64CC::CondCode HeadCmp;
     if (HeadCond.empty() || !parseCond(HeadCond, HeadCmp)) {
       continue;
     }
 
     AArch64CC::CondCode TrueCmp;
     if (TrueCond.empty() || !parseCond(TrueCond, TrueCmp)) {

test/CodeGen/AArch64/combine-comparisons-by-cse.ll

truer:
   call i32 @zoo(i32 42)
   ret void
 
 falser:
   call i32 @zoo(i32 1)
   ret void
 }
 
+define i32 @combine_gt_ge_sel(i64 %v, i64* %p) #0 {
+; CHECK-LABEL: combine_gt_ge_sel
+; CHECK: ldr [[reg1:w[0-9]*]],
+; CHECK: cmp [[reg1]], #0
+; CHECK: csel {{.*}}, gt
+entry:
+  %0 = load i32, i32* @a, align 4
+  %cmp = icmp sgt i32 %0, 0
+  %m = select i1 %cmp, i64 %v, i64 0
+  store i64 %m, i64* %p
+  br i1 %cmp, label %lor.lhs.false, label %land.lhs.true
+
+land.lhs.true:                                    ; preds = %entry
+  %1 = load i32, i32* @b, align 4
+  %2 = load i32, i32* @c, align 4
+  %cmp1 = icmp eq i32 %1, %2
+  br i1 %cmp1, label %return, label %land.lhs.true3
+
+lor.lhs.false:                                    ; preds = %entry
+  %cmp2 = icmp sgt i32 %0, 1
+  br i1 %cmp2, label %land.lhs.true3, label %if.end
+
+land.lhs.true3:                                   ; preds = %lor.lhs.false, %land.lhs.true
+  %3 = load i32, i32* @b, align 4
+  %4 = load i32, i32* @d, align 4
+  %cmp4 = icmp eq i32 %3, %4
+  br i1 %cmp4, label %return, label %if.end
+
+if.end:                                           ; preds = %land.lhs.true3, %lor.lhs.false
+  br label %return
+
+return:                                           ; preds = %if.end, %land.lhs.true3, %land.lhs.true
+  %retval.0 = phi i32 [ 0, %if.end ], [ 1, %land.lhs.true3 ], [ 1, %land.lhs.true ]
+  ret i32 %retval.0
+}
+
 declare i32 @zoo(i32)
 
 declare double @yoo(i32)
 
 declare i32 @xoo(i32, i32)
 
 declare i32 @woo(double, double)