IfConversion: Fix bug introduced by rescanning diamonds.

[iteratee]

Summary

Passing the wrong values for predicate-clobbering. Simple to miss.
Added an assert to make this easier to catch in the future.

[iteratee]

The test case is related, because with the asserts, but without the fix, the test will fail.

It also makes sure that we are correctly re-scanning to find missed opportunities to if-convert.

lib/CodeGen/IfConversion.cpp

1758

This assert would have made the failure occur earlier, making it more obvious what was wrong.

1984

This is the mistake. It should be simple to see that it's correct now.

[davidxl]

I wonder why the problem was not caught by existing tests?

[iteratee]

In D24157#532114, @davidxl wrote:

I wonder why the problem was not caught by existing tests?

In trying to reduce the test case, it seems that pred-clobbering instructions are comparitively rare, except before branches.

It wasn't even caught by any of the buildbots. It showed up compiling openssl for Arm V8 / thumb. It doesn't miscompile for arm V7 / thumb.

In any case, we now have a test that will catch it, and an assert.

[davidxl]

You should probably commit the assertion change indepdendently.

lib/CodeGen/IfConversion.cpp

1758

Perhaps : predicate info can not be clobbered by both sides

1984

Is it because after rescanning the TrueBBI value is not updated?

[iteratee]

lib/CodeGen/IfConversion.cpp

1984

Yes. (Nor should it be)

Those blocks will be scanned for other kinds of if-conversion, and it would be wrong to change TrueBBI and FalseBBI.

[iteratee]

Does this need anything else?

[davidxl]

lgtm

[Eugene.Zelenko]

Looks to be duplicate of D24181.

[iteratee]

In D24157#561476, @Eugene.Zelenko wrote:

Looks to be duplicate of D24181.

Not a duplicate.

This was committed in rL280517

lib/CodeGen/IfConversion.cpp

if (TII->ReverseBranchCondition(RevCond))
     llvm_unreachable("Unable to reverse branch condition!");
   SmallVector<MachineOperand, 4> *Cond1 = &BBI.BrCond;
   SmallVector<MachineOperand, 4> *Cond2 = &RevCond;
 
   // Figure out the more profitable ordering.
   bool DoSwap = false;
   if (TClobbersPred && !FClobbersPred)
     DoSwap = true;
-  else if (TClobbersPred == FClobbersPred) {
+  else if (!TClobbersPred && !FClobbersPred) {
     if (TrueBBI.NonPredSize > FalseBBI.NonPredSize)
       DoSwap = true;
-  }
+  } else if (TClobbersPred && FClobbersPred)
+    llvm_unreachable("Both sides cannot clobber the predicate info.");
   if (DoSwap) {
     std::swap(BBI1, BBI2);
     std::swap(Cond1, Cond2);
   }
 
   // Remove the conditional branch from entry to the blocks.
   BBI.NonPredSize -= TII->RemoveBranch(*BBI.BB);
 
if (!TailBB) {
     if (blockAlwaysFallThrough(TrueBBI))
       TailBB = FalseBBI.TrueBB;
     assert((TailBB || !TrueBBI.IsBrAnalyzable) && "Unexpected!");
   }
 
   if (!IfConvertDiamondCommon(
       BBI, TrueBBI, FalseBBI,
       NumDups1, NumDups2,
-      TrueBBI.ClobbersPred, FalseBBI.ClobbersPred,
+      TClobbersPred, FClobbersPred,
       /* RemoveBranch */ TrueBBI.IsBrAnalyzable,
       /* MergeAddEdges */ TailBB == nullptr))
     return false;
 
   // If the if-converted block falls through or unconditionally branches into
   // the tail block, and the tail block does not have other predecessors, then
   // fold the tail block in as well. Otherwise, unless it falls through to the
   // tail, add a unconditional branch to it.

test/CodeGen/Thumb2/ifcvt-rescan-diamonds.ll

+; RUN: llc -O2 -o - %s | FileCheck %s
+target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "thumbv8-unknown-linux-gnueabihf"
+
+; This is a tricky test case.
+; The point of it is to create a diamond where both the true block and the
+; false block clobber the predicate when viewed as a whole, but only one of them
+; clobbers the predicate when considering the instructions they share.
+
+; Function Attrs: nounwind
+define void @BN_kronecker(i1 %a, i32 %b) #0 {
+entry:
+  br label %while.cond38
+
+while.cond38:                                     ; preds = %if.end111, %entry
+  %cmp79 = icmp eq i32 0, 0
+  br i1 %a, label %cond.true77, label %cond.false87
+
+; CHECK: %cond.true77
+; CHECK-NEXT: @ in Loop
+; CHECK-NEXT: cmp.w {{r[0-9]+}}, #0
+; CHECK-NEXT: it eq
+; CHECK-NEXT: ldreq
+; CHECK-NEXT: it ne
+; CHECK-NEXT: movsne
+; CHECK-NEXT: mvns
+; CHECK-NEXT: b
+cond.true77:                                      ; preds = %while.cond38
+  br i1 %cmp79, label %cond.end84, label %cond.false81
+
+cond.false81:                                     ; preds = %cond.true77
+  %0 = load i32, i32* null, align 4
+  br label %cond.end84
+
+cond.end84:                                       ; preds = %cond.false81, %cond.true77
+  %cond85 = phi i32 [ %0, %cond.false81 ], [ 0, %cond.true77 ]
+  %neg86 = xor i32 %cond85, -1
+  br label %cond.false101
+
+cond.false87:                                     ; preds = %while.cond38
+  br i1 %cmp79, label %cond.false101, label %cond.false91
+
+cond.false91:                                     ; preds = %cond.false87
+  br label %cond.false101
+
+cond.false101:                                    ; preds = %cond.false91, %cond.false87, %cond.end84
+  %cond97 = phi i32 [ %neg86, %cond.end84 ], [ %b, %cond.false91 ], [ 0, %cond.false87 ]
+  %1 = load i32, i32* null, align 4
+  %and106 = and i32 %cond97, %1
+  %and107 = and i32 %and106, 2
+  %tobool108 = icmp ne i32 %and107, 0
+  br i1 %tobool108, label %if.then109, label %if.end111
+
+if.then109:                                       ; preds = %cond.false101
+  store i32 0, i32* undef, align 4
+  br label %if.end111
+
+if.end111:                                        ; preds = %if.then109, %cond.false101
+  %tobool113 = icmp ne i32 0, 0
+  br i1 %tobool113, label %while.cond38, label %end
+
+end:                                              ; preds = %if.end111
+  ret void
+}
+
+attributes #0 = { nounwind }