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

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 13 18:02:23 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).
 
----------------
efriedma wrote:
> 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.
Hmm.  Actually, isSuccessor() isn't sufficient, I think.  Consider the case where the end of the block is unreachable, and the layout successor is an EHPad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79605





More information about the llvm-commits mailing list