[PATCH] D73204: [NFCI][LoopUnrollAndJam] Minor changes.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollAndJam.cpp:525
   if (Count != 1) {
-    SmallVector<DominatorTree::UpdateType, 4> DTUpdates;
-    DTUpdates.emplace_back(DominatorTree::UpdateKind::Delete, ForeBlocksLast[0],
-                           SubLoopBlocksFirst[0]);
-    DTUpdates.emplace_back(DominatorTree::UpdateKind::Delete,
-                           SubLoopBlocksLast[0], AftBlocksFirst[0]);
-
-    DTUpdates.emplace_back(DominatorTree::UpdateKind::Insert,
-                           ForeBlocksLast.back(), SubLoopBlocksFirst[0]);
-    DTUpdates.emplace_back(DominatorTree::UpdateKind::Insert,
-                           SubLoopBlocksLast.back(), AftBlocksFirst[0]);
-    DTU.applyUpdatesPermissive(DTUpdates);
+    DT->changeImmediateDominator(SubLoopBlocksFirst[0], ForeBlocksLast.back());
+    DT->changeImmediateDominator(AftBlocksFirst[0], SubLoopBlocksLast.back());
----------------
Could you provide context on why not using DTU is preferred here? Is it because the existing copy/cloning code in the pass changes the DT in place? I just had a quick look and it looks like the DT is not accessed in between cloning until simplifying the loops after unrolling. 

DTU has the advantage that we can preserve the PDT also for almost no extra cost (just pass a PDT to the DTU additionally). Also, batch updates can be much faster, given that the update set is large enough. 

I think unless there's a strong reason, we should try to replace the existing uses of in-place updates with DTU update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73204