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

[Ruiling, Song via Phabricator via llvm-commits]

ruiling 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
----------------
gandhi21299 wrote:
> 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.
> That is essentially what I have done via breaking `getFallThrough()` and avoiding a call to it.

ok, now that you guys have made the decision like that, feel free to ignore my comments. Sorry I did not follow all the discussion carefully.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134557