[PATCH] D48967: [Dominators] Convert existing passes and utils to use the DomTreeUpdater class

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

Hello @NutshellySima , my comments are inline below. These changes are a good real-world usage of the new DTU class and should help us find any bugs or deficiencies. I'm curious to know what kind of testing you've done on this code: `ninja check`, `test-suite`, and/or `test-suite` for compile time?



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:420
   LoopHeaders.clear();
-  DDT->flush();
+  DTU->getDomTree();
   LVI->enableDT();
----------------
It's not obvious here the reasoning behind this statement. I recommend either changing this to `DTU->flush()` or adding a comment stating the call will force all updates to the internal DT/PDT trees.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:996
       LVI->eraseBlock(SinglePred);
-      MergeBasicBlockIntoOnlyPred(BB, nullptr, DDT);
+      MergeBasicBlockIntoOnlyPred(BB, DTU);
 
----------------
So much cleaner!


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2540
+  bool MadeChange = removeUnreachableBlocks(F, nullptr, &DTU);
+  DTU.getDomTree();
 
----------------
Same as other `flush()` comment.


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:160
   std::vector<DominatorTree::UpdateType> Updates;
-  if (DDT) {
+  if (DTU && DTU->getUpdateStrategy() == DomTreeUpdater::UpdateStrategy::Lazy) {
     Updates.reserve(1 + (2 * succ_size(BB)));
----------------
It might be a good idea to add a comment here why you're only adding these updates for  `UpdateStrategy::Lazy`. Even I can't remember why this is the case, and I did this change. :)

(BTW, this routine is one of the most difficult and I struggled with it for weeks. Keep an eye on changes in here.)


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:198
 
-  // Finally, erase the old block and update dominator info.
-  if (DT)
-    if (DomTreeNode *DTN = DT->getNode(BB)) {
-      DomTreeNode *PredDTN = DT->getNode(PredBB);
-      SmallVector<DomTreeNode *, 8> Children(DTN->begin(), DTN->end());
-      for (DomTreeNode *DI : Children)
-        DT->changeImmediateDominator(DI, PredDTN);
-
-      DT->eraseNode(BB);
-    }
-
   if (LI)
     LI->removeBlock(BB);
----------------
Why did you move the `LI` and `MemDep` calls up?


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:206
+  if (DTU) {
+    if (DTU->getUpdateStrategy() == DomTreeUpdater::UpdateStrategy::Eager) {
+      if (DTU->hasDomTree()) {
----------------
I'm seeing several statements of this long form. Maybe add query methods like `isUpdateEager()` and `isUpdateLazy()` to simplify long comparison statements like these?


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:223
+      }
+    } else
+      DTU->applyUpdates(Updates);
----------------
Please add `{}` here. Even though it's only one statement the if section uses them and it's a long block.  Putting a small amount of distance between the two next statements makes it clear that we always call `deleteBB`. 

You might also want to add a comment here denoting this as the `Lazy` path. With the outer `if/else` it's a little hard to read.


================
Comment at: lib/Transforms/Utils/Local.cpp:746
+        // we recalculate the entire tree.
+        DTU->recalculate(*(DestBB->getParent()));
+      else
----------------
Have you verified this does not need to be done for the `Eager` strategy? From what I can tell no one in LLVM is calling to `RemovePredecessorAndSimplify`. I wouldn't have added support for DDT here unless I absolutely needed it so I suspect the changes to JumpThreading have removed the need for it.   I am nervous about this change because I am uncertain it's correct.


================
Comment at: lib/Transforms/Utils/Local.cpp:1062
+  // Zap all the instructions in the block.
+  while (!BB->empty()) {
+    Instruction &I = BB->back();
----------------
Why are you always zapping the block here instead of relying on the `deleteBB` routine and the knowledge of your current `UpdateStrategy` within the `DTU` class?


================
Comment at: lib/Transforms/Utils/Local.cpp:1890
 
 static bool markAliveBlocks(Function &F,
+                            SmallPtrSetImpl<BasicBlock *> &Reachable,
----------------
I have concerns about code coverage here (and the sole user of this routine, `removeUnreachableBlocks` below). JumpThreading no longer makes use of this routine and the sole user of it now is `RewriteStatepointsForGC.cpp`. I don't know how much actual testing of your new code paths is actually happening.


================
Comment at: lib/Transforms/Utils/Local.cpp:2126
+      ToDeleteBBs.push_back(BB);
+      // Zap all the instructions in the block.
+      while (!BB->empty()) {
----------------
Same question here as the similar zap blocks comment above.


Repository:
  rL LLVM

https://reviews.llvm.org/D48967