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

[Kyle Butt via llvm-commits]

iteratee added inline comments.

================
Comment at: lib/CodeGen/IfConversion.cpp:1815-1820
@@ -1782,4 +1814,8 @@
 
-  if (RemoveTrueBranch)
-    BBI1->NonPredSize -= TII->RemoveBranch(*BBI1->BB);
+  // The branches have been checked to match, so it is safe to remove the branch
+  // in BB1 and rely on the copy in BB2
+#ifndef NDEBUG
+  verifySameBranchInstructions(&MBB1, &MBB2);
+#endif
+  BBI1->NonPredSize -= TII->RemoveBranch(*BBI1->BB);
   // Remove duplicated instructions.
----------------
davidxl wrote:
> Is it an expectation later that the terminator instruction (in two BBs) are either both removed or they both exist -- i.e. what verifySameInstructions does?
> 
> You patch looks right, but can you check why the problem does not appear before ? Is it because NumDups2 computation is different?
They are expected to match except for a possible unconditional branch, and they may be reversed from each other.
If they are unanalyzable, they must match exactly.

It didn't occur because by rescanning, we find new opportunities to if-convert.

Previously if the branch instruction were unpredicable, the block was not considered a candidate for diamond if-conversion. Now, as long as the branch instruction at the end of the 2 blocks is the same, we are willing to diamond convert.

That means we now miss the case of a "diamond" where both sides end in an unanalyzable but predicable branch.

================
Comment at: lib/CodeGen/IfConversion.cpp:1843
@@ +1842,3 @@
+    BBI2->NonPredSize -= TII->RemoveBranch(*BBI2->BB);
+  else {
+    do {
----------------
davidxl wrote:
> Why not always skip it?
For analyzable branches, the actual branch instruction may not be necessary. e.g. a diamond, where the join block is either merged, or a fallthrough. In that case, the caller will handle the branch.


https://reviews.llvm.org/D23939