[PATCH] D23939: IfConversion: Fix branch predication bug.

[Kyle Butt via llvm-commits]

iteratee created this revision.
iteratee added a reviewer: kparzysz.
iteratee added a subscriber: llvm-commits.
iteratee set the repository for this revision to rL LLVM.

This bug shows up with diamonds that share unpredicable, unanalyzable branches.
There's an included test case from Hexagon. What was happening was that we were
attempting to predicate the branch instruction despite the fact that it was
checked to be the same. Now for unanalyzable branches we skip over the branch
instructions when predicating the block.


Repository:
  rL LLVM

https://reviews.llvm.org/D23939

Files:
  lib/CodeGen/IfConversion.cpp

Index: lib/CodeGen/IfConversion.cpp
===================================================================
--- lib/CodeGen/IfConversion.cpp
+++ lib/CodeGen/IfConversion.cpp
@@ -248,8 +248,7 @@
     bool IfConvertDiamondCommon(BBInfo &BBI, BBInfo &TrueBBI, BBInfo &FalseBBI,
                                 unsigned NumDups1, unsigned NumDups2,
                                 bool TClobbersPred, bool FClobbersPred,
-                                bool RemoveTrueBranch, bool RemoveFalseBranch,
-                                bool MergeAddEdges);
+                                bool RemoveBranch, bool MergeAddEdges);
     bool IfConvertDiamond(BBInfo &BBI, IfcvtKind Kind,
                           unsigned NumDups1, unsigned NumDups2,
                           bool TClobbers, bool FClobbers);
@@ -1678,19 +1677,16 @@
 ///               and FalseBBI
 /// \p NumDups2 - number of shared instructions at the end of \p TrueBBI
 ///               and \p FalseBBI
-/// \p RemoveTrueBranch - Remove the branch of the true block before predicating
-///                       Only false for unanalyzable fallthrough cases.
-/// \p RemoveFalseBranch - Remove the branch of the false block before
-///                        predicating Only false for unanalyzable fallthrough
-///                        cases.
+/// \p RemoveBranch - Remove the common branch of the two blocks before
+///                   predicating. Only false for unanalyzable fallthrough
+///                   cases. The caller will replace the branch if necessary.
 /// \p MergeAddEdges - Add successor edges when merging blocks. Only false for
 ///                    unanalyzable fallthrough
 bool IfConverter::IfConvertDiamondCommon(
     BBInfo &BBI, BBInfo &TrueBBI, BBInfo &FalseBBI,
     unsigned NumDups1, unsigned NumDups2,
     bool TClobbersPred, bool FClobbersPred,
-    bool RemoveTrueBranch, bool RemoveFalseBranch,
-    bool MergeAddEdges) {
+    bool RemoveBranch, bool MergeAddEdges) {
 
   if (TrueBBI.IsDone || FalseBBI.IsDone ||
       TrueBBI.BB->pred_size() > 1 || FalseBBI.BB->pred_size() > 1) {
@@ -1727,7 +1723,6 @@
   if (DoSwap) {
     std::swap(BBI1, BBI2);
     std::swap(Cond1, Cond2);
-    std::swap(RemoveTrueBranch, RemoveFalseBranch);
   }
 
   // Remove the conditional branch from entry to the blocks.
@@ -1780,8 +1775,7 @@
   BBI.BB->splice(BBI.BB->end(), &MBB1, MBB1.begin(), DI1);
   MBB2.erase(MBB2.begin(), DI2);
 
-  if (RemoveTrueBranch)
-    BBI1->NonPredSize -= TII->RemoveBranch(*BBI1->BB);
+  BBI1->NonPredSize -= TII->RemoveBranch(*BBI1->BB);
   // Remove duplicated instructions.
   DI1 = MBB1.end();
   for (unsigned i = 0; i != NumDups2; ) {
@@ -1799,11 +1793,18 @@
   // must be removed.
   RemoveKills(MBB1.begin(), MBB1.end(), DontKill, *TRI);
 
-  // Remove 'false' block branch, and find the last instruction to predicate.
-  // Save the debug location.
-  if (RemoveFalseBranch)
-    BBI2->NonPredSize -= TII->RemoveBranch(*BBI2->BB);
   DI2 = BBI2->BB->end();
+  // The branches have been checked to match. Skip over the branch in the false
+  // block so that we don't try to predicate it.
+  if (RemoveBranch)
+    BBI2->NonPredSize -= TII->RemoveBranch(*BBI2->BB);
+  else {
+    do {
+      assert(DI2 != MBB2.begin());
+      DI2--;
+    } while (DI2->isDebugValue() || DI2->isBranch());
+    DI2++;
+  }
   while (NumDups2 != 0) {
     // NumDups2 only counted non-dbg_value instructions, so this won't
     // run off the head of the list.
@@ -1901,8 +1902,7 @@
       BBI, TrueBBI, FalseBBI,
       NumDups1, NumDups2,
       TClobbersPred, FClobbersPred,
-      /* RemoveTrueBranch */ true, /* RemoveFalseBranch */ true,
-      /* MergeAddEdges */ true))
+      /* RemoveBranch */ true, /* MergeAddEdges */ true))
     return false;
 
   // Add back the branch.
@@ -1939,8 +1939,7 @@
       BBI, TrueBBI, FalseBBI,
       NumDups1, NumDups2,
       TrueBBI.ClobbersPred, FalseBBI.ClobbersPred,
-      /* RemoveTrueBranch */ TrueBBI.IsBrAnalyzable,
-      /* RemoveFalseBranch */ FalseBBI.IsBrAnalyzable,
+      /* RemoveBranch */ TrueBBI.IsBrAnalyzable,
       /* MergeAddEdges */ TailBB == nullptr))
     return false;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D23939.69413.patch
Type: text/x-patch
Size: 4163 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160826/8a009abc/attachment.bin>