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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks, I'll address the comments on Monday.



================
Comment at: include/llvm/Transforms/Utils/Cloning.h:270
+                                                ValueToValueMapTy &ValueMapping,
+                                                DomTreeUpdater &DTU);
+
----------------
NutshellySima wrote:
> I think it's better to use a pointer to `DomTreeUpdater` here since DTU is not a must to use this function.
I can change it, I just figured given the few users it might be better to require DTU.


================
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.
----------------
NutshellySima wrote:
> You need to be careful that `BB` isn't erased immediately because you use `UpdateStrategy::Lazy`.
Right, I will update the comment. Having the blocks deleted at the end of this function (or when getDomTree() is called) should be fine.


================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:815
+  //        in the update set here.
+  DTU.applyUpdates({{DominatorTree::Delete, PredBB, BB},
+                    {DominatorTree::Insert, PredBB, NewBB},
----------------
NutshellySima wrote:
> 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  
I basically took the updates from JumpThreading. I'll take another look on Monday.


https://reviews.llvm.org/D54312