[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:
> > bcahoon wrote:
> > > 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:
> > > 
> > > 
> > That makes sense... but I'm not sure checking for `PrevBB->terminators().empty()` is the right way to fix that.  You can have fallthrough with terminators.  For example:
> > 
> > ```
> > BB1:  
> >   other instructions
> >   s_cbranch_execnz BB3
> > BB2:
> > ```
> In the example, a branch won't be inserted which is what we expect right? Or am I missing something?
A branch is needed because a new block is inserted between BB1 and BB2 that shouldn't be executed when control goes from BB1 -> BB2.  So, after the transformation,

  BB1:
    other instructions
    s_cbranch_execnz BB3
    s_branch BB2
  BB4:

  BB2:



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134557