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

[Chijun Sima via Phabricator via llvm-commits]

NutshellySima added a comment.

In https://reviews.llvm.org/D48967#1154447, @brzycki wrote:

> 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?


I ran `ninja check-all` with assertions enabled.



================
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);
----------------
brzycki wrote:
> Why did you move the `LI` and `MemDep` calls up?
Previously, this function accepts `DT/DDT`. In the `DT` case, it calls `DT->eraseNode(BB)` then call `BB->eraseFromParent()` after calling `LI->removeBlock(BB)`; In the `DDT` case, it calls `DDT.deleteBB(BB)` before calling `LI->removeBlock(BB)`.

Since outside users can pass a `DTU` working under Eager UpdateStrategy (that is to say, calling `DTU.deleteBB(BB)` will immediately do both `DT/PDT.eraseNode(BB)` and `BB->eraseFromParent()`) which obviously has conflicts with `LI->removeBlock(BB)`, so I move them up.


================
Comment at: lib/Transforms/Utils/Local.cpp:746
+        // we recalculate the entire tree.
+        DTU->recalculate(*(DestBB->getParent()));
+      else
----------------
brzycki wrote:
> 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.
This part of the code reuses the DT handling code before to be the Eager UpdateStrategy ones and the DDT handling code to the Lazy ones. The only perhaps uncertain change is whether my PDT handling code under Eager strategy (L729-L737) is correct and whether the previous code of DDT can also handle the PDT case.
I modified the unittest of local.cpp to cover the change and it seems correct.


================
Comment at: lib/Transforms/Utils/Local.cpp:1062
+  // Zap all the instructions in the block.
+  while (!BB->empty()) {
+    Instruction &I = BB->back();
----------------
brzycki wrote:
> 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?
I'm quite uncertain here because I have to call `deleteBB` after `applyUpdates` to make this utility works under Eager strategy. But it asserts when running the unittest, so I add these zapping block codes here.


Repository:
  rL LLVM

https://reviews.llvm.org/D48967