[MBB] add a missing corner case in UpdateTerminator()

[haicheng]

Summary

If the layout after the block placement is like this

BB#8: 
        ......
        JGE_1 <BB#9>, %EFLAGS<imp-use>
    Successors according to CFG: BB#9(0x80000000 / 0x80000000 = 100.00%)

BB#10:
        ......

where BB#8 ends with a conditional branch, but the next block is not its successor. The conditional branch should be changed to unconditional branch in MachineBasicBlock::UpdateTerminator(), but this case is missing.

In cases like above example, the results returned by AnalyzeBranch() are: TBB = branch target; FBB = nullptr; Cond = branch condition (not empty)

This corner cases happens if a block (e.g. BB#8) branches and fallthourghs into the same block (e.g. BB#9) before the block placement, and the block placement moves this successor (e.g. BB#9) somewhere else.

[haicheng]

update the comment.

[uabelho]

This looks good to me.
It solves the problem we've seen and I haven't noticed anything bad during testing.

[majnemer]

LGTM

llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp

void MachineBasicBlock::updateTerminator() {
   MachineBasicBlock *FallthroughBB = nullptr;
   for (succ_iterator SI = succ_begin(), SE = succ_end(); SI != SE; ++SI) {
     if ((*SI)->isEHPad() || *SI == TBB)
       continue;
     assert(!FallthroughBB && "Found more than one fallthrough successor.");
     FallthroughBB = *SI;
   }
 
-  if (!FallthroughBB && canFallThrough()) {
+  if (!FallthroughBB) {
+    if (canFallThrough()) {
       // We fallthrough to the same basic block as the conditional jump targets.
       // Remove the conditional jump, leaving unconditional fallthrough.
       // FIXME: This does not seem like a reasonable pattern to support, but it
       // has been seen in the wild coming out of degenerate ARM test cases.
       TII->RemoveBranch(*this);
   
       // Finally update the unconditional successor to be reached via a branch if
       // it would not be reached by fallthrough.
       if (!isLayoutSuccessor(TBB))
         TII->InsertBranch(*this, TBB, nullptr, Cond, DL);
       return;
     }
 
+    // We enter here iff exactly one successor is TBB which cannot fallthrough
+    // and the rest successors if any are EHPads.  In this case, we need to
+    // change the conditional branch into unconditional branch.
+    TII->RemoveBranch(*this);
+    Cond.clear();
+    TII->InsertBranch(*this, TBB, nullptr, Cond, DL);
+    return;
+  }
+
   // The block has a fallthrough conditional branch.
   if (isLayoutSuccessor(TBB)) {
     if (TII->ReverseBranchCondition(Cond)) {
       // We can't reverse the condition, add an unconditional branch.
       Cond.clear();
       TII->InsertBranch(*this, FallthroughBB, nullptr, Cond, DL);
       return;
     }

llvm/trunk/test/CodeGen/X86/update-terminator.mir

+# RUN: llc -march=x86-64 -verify-machineinstrs -run-pass block-placement -o /dev/null %s 2>&1 | FileCheck %s
+# Check the conditional jump in bb.1 is changed to unconditional after block placement swaps bb.2 and bb.3.
+
+--- |
+  @a = external global i16
+  @b = external global i32
+
+  ; Function Attrs: nounwind
+  define void @f2() {
+    br i1 undef, label %bb1, label %bb3
+
+  bb1:
+    br i1 undef, label %bb2, label %bb2
+
+  bb2:
+    br label %bb4
+
+  bb3:
+    br label %bb2
+
+  bb4:
+    ret void
+  }
+
+
+...
+---
+# CHECK-LABEL: name: f2
+# CHECK: bb.1:
+# CHECK: JMP_1 %bb.2
+# CHECK: bb.3:
+# CHECK: bb.2:
+name:            f2
+body:             |
+  bb.0 (%ir-block.0):
+    successors: %bb.1(50), %bb.3(50)
+
+    JNE_1 %bb.1, implicit %eflags
+    JMP_1 %bb.3
+  bb.1:
+    successors: %bb.2(100)
+
+    JNE_1 %bb.2, implicit %eflags
+
+  bb.2:
+    successors: %bb.4(100)
+
+    JMP_1 %bb.4
+
+  bb.3:
+    successors: %bb.2(100)
+    JMP_1 %bb.2
+
+  bb.4:
+    RETQ
+
+...