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

[James Y Knight via Phabricator via llvm-commits]

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

> Is the endgame here that we relax the restriction that updateTerminator() requires an analyzable branch?

That wasn't my goal. My goal is to remove the assumption that it's possible to reconstruct which branches should be present at the end of the block, based purely on CFG successors. Once upon a time, that may have been a simple prospect -- e.g. if there's a conditional branch, there should be 2 CFG successors, and at least one must have an actual branch instruction, so you just add/remove branches to the other one, depending on whether it's the fallthrough block. But then we added exception handling, and the EHPad successors mess up that simplicity. But, we could at least filter out EHPad blocks, because those can't also be the target of a normal jump.

However, I'm now proposing to make inlineasm_br not be a terminator instruction, which means the block can have extra successors in a new way. And unlike the EHPad blocks, the targets of an INLINEASM_BR may _also_ be used as normal jump/fallthroughs of another block.

I split this out as a separate review, because I think it's actually better that this is less magical and more explicit, regardless of whether we decide to make that INLINEASM_BR change.



================
Comment at: llvm/lib/CodeGen/TailDuplicator.cpp:955
+    if (ShouldUpdateTerminators)
+      PrevBB->updateTerminator(TailBB->getNextNode());
+
----------------
efriedma wrote:
> Is this related to the rest of the patch?  I don't see any obvious interaction.
This code previously left the block in a bogus state, where the new CFG was not properly reflected in the instructions in the block (e.g. the machine instructions could fallthrough to the next block, while the CFG said it should actually jump somewhere else).

In the past, other future calls to updateTerminator managed to reconstruct the desired state of things, and insert the correct jump, based on the CFG successor. That no longer happens magically, so this code now must ensure that it has inserted a jump at the end of the block if it intends there to be one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79605