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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

There are many places in this patch that just gives the layout successor as an argument to `updateTerminator`, which does not consider EH pad possibility. I think the main reason we enumerated the successor iterator is to filter out EH pads, and I'm not sure if deleting that and delegating the task of giving the right layout successor to the caller side is a good idea, because each caller has to evaluate that separately.
Also 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, after calling `updateTerminator`, we may not end up falling through to the `PreviousLayoutSuccessor` variable.



================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:526
+  /// may now be able to fallthrough to the current layout successor.
+  void updateTerminator(MachineBasicBlock *PreviousLayoutSuccessor);
 
----------------
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..?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79605