[PATCH] D79605: MachineBasicBlock::updateTerminator now requires an explicit layout successor.

Wed May 13 12:31:35 PDT 2020

efriedma added inline comments.

================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:586
+      // The block has an unconditional fallthrough (or the end of the block is
+      // unreachable).

----------------
jyknight wrote:
> efriedma wrote:
> > "or the end of the block is unreachable" makes me a little worried.  If I'm understanding correctly, if do somehow end up in that situation, the layout successor won't actually be a successor of the BasicBlock, and `assert(isSuccessor(PreviousLayoutSuccessor))` will fail.
> Um, yup. I think you've identified a serious issue.
> 
> Identifying whether the instruction stream represents a fallthrough is another place where we look at successor list to help figure out what the instructions mean. That's something else I should fix -- we currently don't actually represent the tail of the block being unreachable in the instruction stream. IMO, this is a mistake, and we ought to have an UNREACHABLE_MARKER pseudo-inst to represent this situation, and stop depending on querying the successors list to figure this out.
> 
> For the purposes of this patch and making incremental progress, I believe it's sufficient to just delete the assert and go with `if(!PreviousLayoutSuccessor || !isSuccessor(PreviousLayoutSuccessor)) return;`. That won't fully achieve my goal, but I'll work on the above idea coupled with a cleanup of all the fallthrough handling, here and everywhere else, as a separate change.
Yes, I think that works.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79605