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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:443
+
+static PredsWithCondsTy tryToSplitOnPHIPredicatedArgument(CallSite CS) {
   if (!isPredicatedOnPHI(CS))
----------------
junbuml wrote:
> Now this function collects PredsWithCondsTy without performing splitting. Can you change this function name accordingly. 
I've also added short comments.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:453
+                                                       DomTreeUpdater &DTU) {
   auto Preds = getTwoPredecessors(CS.getInstruction()->getParent());
   if (Preds[0] == Preds[1])
----------------
NutshellySima wrote:
> I recommend adding `assert(DTU.hasDomTree() && ...)` here.
I've reverted the patch using the DT here, because it depends on the current patch. So this code is gone in the latest revision.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:499-503
+  auto PredsWithConds = tryToSplitOnPredicatedArgument(CS, DTU);
+  if (PredsWithConds.empty())
+    PredsWithConds = tryToSplitOnPHIPredicatedArgument(CS);
+  if (PredsWithConds.empty())
+    return false;
----------------
junbuml wrote:
> What about doing this in a separate function ?
You mean calling the 2 functions above in a separate function? Given that tryToSplitCallSite itself is quite small, I think it is clearer to have to calls here directly. 


================
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:
> 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).
> 
> 
It looks like `DuplicateInstructionsInSplitBetween` should only be called if there is a single edge between PredBB and BB, otherwise it is not really clear what the return value should be. I have added an assertion to make sure there is only one edge between PredBB and BB (and it holds for a bootstrap build & the test suite). IIUC, this should be enough to address the issue mentioned earlier?




================
Comment at: unittests/Transforms/Utils/CloningTest.cpp:229
   ValueToValueMapTy Mapping;
+  DomTreeUpdater DTU(DomTreeUpdater::UpdateStrategy::Lazy);
 
----------------
NutshellySima wrote:
> 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.
I've added a comment to make clear that the DTU here is just a dummy, as the current tests do not test DT preservation. I suppose the  existing JT & CSP test should cover DT preservation, but I would be happy to extend the tests in a separate patch if you think that is valuable.


https://reviews.llvm.org/D54312