[PATCH] D92200: Ensure SplitEdge to return the new block between the two given blocks

[Bangtian Liu via Phabricator via llvm-commits]

kobeliu85 added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:512
     assert(SP == BB && "CFG broken");
     SP = nullptr;
+    return SplitBlock(Succ, &Succ->front(), DT, LI, MSSAU, "", /*Before=*/true);
----------------
hoy wrote:
> Nit: Is resetting `SP` still needed?
I also don't understand why it is needed in the first place, and it is from the original code, so I didn't want to change it.  
Whether do you know about it and if so could you explain why?


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:595
+  if (DT) {
+    DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Lazy);
+    SmallVector<DominatorTree::UpdateType, 8> DTUpdates;
----------------
hoy wrote:
> Can we just change the dominator tree edges directly instead of going through the dominator updater?
> 
> like 
> 
> ```
> OldIdom = OldNode->getIDom();
> DT->changeImmediateDominator(OldNode, NewNode);
> DT->changeImmediateDominator(NewNode, OldIdom)
> ```
> 
> 
 DomTreeUpdater provides a uniform way to update dominator tree related data structures, e.g. DT, MSSA, PDT.
My first version implementation modified the DT directly and then switched to DTUpdater by following the suggestion from another reviewer. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92200