[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 inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:526
+  /// may now be able to fallthrough to the current layout successor.
+  void updateTerminator(MachineBasicBlock *PreviousLayoutSuccessor);
 
----------------
aheejin wrote:
> jyknight wrote:
> > aheejin wrote:
> > > I'm not very sure `PreviousLayoutSuccessor` stands for. `updateTerminator` does not change layout (= order of BBs), so the layout successor should stay the same before and after calling `updateTerminator`. Depending on branches created, after calling `updateTerminator`, we may not end up falling through to the `PreviousLayoutSuccessor` variable, but in that case it still succeeds MBB in layout. `PreviousLayoutSuccessor`, to me, sounds like the layout successor will change after calling this function. How about just `LayoutSuccessor` or something..?
> > In some (but not all) of the callers, the current actual block ordering of the function has been modified _before_ calling this function. Therefore, the argument passed as "PreviousLayoutSuccessor" is no longer the actual, current, layout successor, at the time of this call.
> > 
> > If there is currently a fallthrough at the end of the block, a branch may need to be inserted, and the target of that branch is PreviousLayoutSuccessor.
> > 
> Then the change in WebAssemblyCFGSort.cpp can be incorrect. What you gave as an argument there is the 'current' layout successor. The order change happens in the caller, `SortBlocks()`, and you have to track all changes there to figure out what the previous layout successor had been.
> 
> I'm still not sure if this approach, that pushing all the responsibilities of figuring out "previous layout successor" to the callers, is feasible or scalable. The order change can happen anywhere, sometimes not right before you call `MachineBasicBlock::updateTerminator()`.
The change in WebAssemblyCFGSort.cpp is passing the original/correct successor, because it's looking it up based on the block id created in RenumberBlocks (at the top of SortBlocks), which is before the blocks are reordered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79605