[PATCH] D134557: [BranchRelaxation] Fall through only if block has no unconditional branches

[Anshil Gandhi via Phabricator via llvm-commits]

gandhi21299 added inline comments.


================
Comment at: llvm/lib/CodeGen/BranchRelaxation.cpp:500
     MachineBasicBlock *PrevBB = &*std::prev(DestBB->getIterator());
-    if (auto *FT = PrevBB->getFallThrough()) {
-      assert(FT == DestBB);
-      TII->insertUnconditionalBranch(*PrevBB, FT, DebugLoc());
-      // Recalculate the block size.
-      BlockInfo[PrevBB->getNumber()].Size = computeBlockSize(*PrevBB);
+    bool FailToAnalyze = TII->analyzeBranch(*PrevBB, TBB, FBB, Cond);
+    // Fall through only if PrevBB has no unconditional branch as one of its
----------------
ruiling wrote:
> I am not quite sure whether a simplified approach would work here: Check whether there is explicit branch to `FT` in `PrevBB`? if no, insert unconditional branch at the end of `PrevBB` to `FT`, otherwise do nothing. Does this solve the problem here? The remaining question is how to detect whether there is pre-existing branch to `FT` in `PrevBB`? I think we can do an iteration over the terminators of `PrevBB` to find if there is any branch instruction that has a MBB operand referencing `DestBB`.
> The overall idea would look like:
> ```
> if ((auto *FT = PrevBB->getFallThrough()) {
>   iterating over the terminators of PrevBB, see if there is any branch instruction which has a MachineOperand that references FT
>   if (explicit branch to FT NOT found)
>     insert unconditional branch and recalculate block size
> }
> ```
> Does this sounds good to you? @efriedma @arsenm 
That is essentially what I have done via breaking `getFallThrough()` and avoiding a call to it. The problem with `getFallThrough()`, as discussed previously, is that it consists of confusing logic. I use `analyzeBranch()` to determine the successors of `PrevBB`, which is the same as iterating over terminators and checking their operands.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134557/new/

https://reviews.llvm.org/D134557