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

[Hongtao Yu via Phabricator via llvm-commits]

hoy 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);
----------------
kobeliu85 wrote:
> 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?
Oh, it's probably there to suppress compiler waning of unused locals in release build since the above assert is debug-only.


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:595
+  if (DT) {
+    DomTreeUpdater DTU(DT, DomTreeUpdater::UpdateStrategy::Lazy);
+    SmallVector<DominatorTree::UpdateType, 8> DTUpdates;
----------------
kobeliu85 wrote:
> 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. 
I see. It makes sense to use the unified API for better maintenance though I thought it might cause a little bit performance loss.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92200