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

[Chijun Sima via Phabricator via llvm-commits]

NutshellySima added inline comments.


================
Comment at: lib/Transforms/Utils/CloneFunction.cpp:815
+  //        in the update set here.
+  DTU.applyUpdates({{DominatorTree::Delete, PredBB, BB},
+                    {DominatorTree::Insert, PredBB, NewBB},
----------------
fhahn wrote:
> 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.
You can also use `DTU.applyUpdates(..., ForceRemoveDuplicate=true)` here to automatically remove this invalid update under `Eager` UpdateStrategy. But it is a side-effect of deduplication which seems like a hack and I do not recommend using it :\. (JT uses `Lazy` UpdateStrategy which automatically does updates deduplication).




https://reviews.llvm.org/D54312