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

[Brendon Cahoon via Phabricator via llvm-commits]

bcahoon added inline comments.


================
Comment at: llvm/lib/CodeGen/BranchRelaxation.cpp:485
     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);
+    if (PrevBB->terminators().empty()) {
+      if (auto *FT = PrevBB->getFallThrough()) {
----------------
gandhi21299 wrote:
> efriedma wrote:
> > gandhi21299 wrote:
> > > arsenm wrote:
> > > > I'd expect for this to be covered by getFallThrough check for false. Did something weird happen with an unanalyzable branch?
> > > We found a case where fixupUnconditionalBranch() appends a s_branch X to a block with terminators s_cbranch X, s_branch Y.
> > There's a comment in getFallThrough: "If there is some explicit branch to the fallthrough block, it can obviously reach, even though the branch should get folded to fall through implicitly."  Maybe that bit of code is what's confusing the logic here?
> > 
> > (I'm not sure why that logic exists...)
> Yea there is no check for the "explicit branch" or any form of branch at all.
> There's a comment in getFallThrough: "If there is some explicit branch to the fallthrough block, it can obviously reach, even though the branch should get folded to fall through implicitly."  Maybe that bit of code is what's confusing the logic here?
> 
Yes - this is the code that causes the problem.  I'm also not sure why that logic is there, though it's been around since 2009. 

The basic block where getFallThrough returns true contains the following branches:

  BB1:
    other instructions
    s_cbranch_execnz BB2
    s_branch BB3
  BB2:




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134557