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

[James Y Knight via Phabricator via llvm-commits]

jyknight marked 3 inline comments as done.
jyknight added a comment.

> Are you planning to land this or D79793 <https://reviews.llvm.org/D79793> is the alternative approach?

They're not alternatives, I plan to land both.



================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:586
+      // The block has an unconditional fallthrough (or the end of the block is
+      // unreachable).
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79605