[PATCH] D54312: [CSP, Cloning] Update DuplicateInstructionsInSplitBetween to use DomTreeUpdater.

[Chijun Sima via Phabricator via llvm-commits]

NutshellySima added inline comments.


================
Comment at: include/llvm/Transforms/Utils/Cloning.h:270
+                                                ValueToValueMapTy &ValueMapping,
+                                                DomTreeUpdater &DTU);
+
----------------
I think it's better to use a pointer to `DomTreeUpdater` here since DTU is not a must to use this function.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:453
+                                                       DomTreeUpdater &DTU) {
   auto Preds = getTwoPredecessors(CS.getInstruction()->getParent());
   if (Preds[0] == Preds[1])
----------------
I recommend adding `assert(DTU.hasDomTree() && ...)` here.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:531
 
       // Successful musttail call-site splits result in erased CI and erased BB.
       // Check if such path is possible before attempting the splitting.
----------------
You need to be careful that `BB` isn't erased immediately because you use `UpdateStrategy::Lazy`.


================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:815
+  //        in the update set here.
+  DTU.applyUpdates({{DominatorTree::Delete, PredBB, BB},
+                    {DominatorTree::Insert, PredBB, NewBB},
----------------
I have concerns on correctness here. It seems that [[ https://github.com/llvm-mirror/llvm/blob/6b4d662418d2475d282e3b2f309798b210d6a666/lib/Transforms/Utils/BreakCriticalEdges.cpp#L223-L224 | llvm::SplitCriticalEdge ]] used inside `SplitEdge` sometimes does not delete all edges between `PredBB` and `BB`. ([[ https://github.com/llvm-mirror/llvm/blob/6b4d662418d2475d282e3b2f309798b210d6a666/lib/Transforms/Utils/BreakCriticalEdges.cpp#L187 | More info ]])
And maybe it's better to do insertions then do deletions like [[ https://github.com/llvm-mirror/llvm/blob/6b4d662418d2475d282e3b2f309798b210d6a666/lib/Transforms/Utils/BreakCriticalEdges.cpp#L217-L219 | this ]]. It seems like a performance related consideration? @kuhar  


================
Comment at: unittests/Transforms/Utils/CloningTest.cpp:229
   ValueToValueMapTy Mapping;
+  DomTreeUpdater DTU(DomTreeUpdater::UpdateStrategy::Lazy);
 
----------------
The unittest is not effective. Neither `DT` nor `PDT` is passed into `DomTreeUpdater`. And `UpdateStrategy::Eager` also needs testing.

Both `DomTreeUpdater DTU(NotNullDT, NotNullPDT, Lazy)` and  `DomTreeUpdater DTU(NotNullDT, NotNullPDT, Eager)` need to be tested.


https://reviews.llvm.org/D54312