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

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

In https://reviews.llvm.org/D48967#1154501, @NutshellySima wrote:

> I ran `ninja check-llvm` with assertions enabled.


That is definitely not sufficient. The .ll test cases are only a small subset of real-world code and people only add to them to verify a new feature or to prove a bug is fixed.  I recommend running `test-suite` to get a better representation of code out in the world.  If you are uncertain how to do so please e-mail me and I'll provide detailed instructions. I'm hoping we can avoid stressful bug report debugging and LLVM code reverts/reapplies.



================
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);
----------------
NutshellySima wrote:
> 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.
That makes sense. Thanks!


================
Comment at: lib/Transforms/Utils/Local.cpp:746
+        // we recalculate the entire tree.
+        DTU->recalculate(*(DestBB->getParent()));
+      else
----------------
NutshellySima wrote:
> 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.
Understood. One way I verified coverage during testing was adding `assert(0 && "tracking string 1")` on code paths I was uncertain were being exercised. After running `ninja check` and `test-suite` I'd `grep` for the string.


================
Comment at: lib/Transforms/Utils/Local.cpp:1062
+  // Zap all the instructions in the block.
+  while (!BB->empty()) {
+    Instruction &I = BB->back();
----------------
NutshellySima wrote:
> 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.
Your response makes me even less comfortable with this code. Users will not be aware this zapping is to prepare the code for DTU eager updates and it's also done on every block, even if a DTU isn't passed in which is unnecessary.  I recommend debugging this more to see if you can find a way to hide this kind of BB manipulation and tie it to the eager strategy better.


Repository:
  rL LLVM

https://reviews.llvm.org/D48967