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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin 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);
 
----------------
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()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79605