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

169–181

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

363–367

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

case AArch64::ADDSXri: {
         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;
       }
+      // Check if there are other uses (e.g. CSEL) of flag reg.
+      for(auto &MO : I->implicit_operands()) {
+        if (!MO.isReg() || !MO.isDef())
+          continue;
+        for(auto SuccBB : MBB->successors())
+          if (SuccBB->isLiveIn(MO.getReg()))
+            return nullptr;
+        for(auto II = std::next(I); II != MBB->end(); ++II) {
+          if (II->getOpcode() == AArch64::Bcc)
+            continue;
+          if (II->readsRegister(MO.getReg()))
+            return nullptr;
+        }
+      }
       return I;
     }
     // Prevent false positive case like:
     // cmp      w19, #0
     // cinc     w0, w19, gt
     // ...
     // fcmp     d8, #0.0
     // b.gt     .LBB0_5
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)