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

[Chijun Sima via Phabricator via llvm-commits]

NutshellySima marked 7 inline comments as done.
NutshellySima added inline comments.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:420
   LoopHeaders.clear();
-  DDT->flush();
+  DTU->getDomTree();
   LVI->enableDT();
----------------
brzycki wrote:
> 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.
I think `DTU->flush()` should only be called when people need either
1. Flush all deleteBB awaiting deletion.
2. Do queries on both DT and PDT.

In the future, if DTU holds both DT and PDT in JumpThreading, call `DTU->getDomTree` which only flushes DT can pend updates on PDT as lazy as possible.


================
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)));
----------------
brzycki wrote:
> 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.)
OK. The reason why only these updates are only added for `UpdateStrategy::Lazy` is that I reuse the codes handling DT previously to handle the `Eager` case. If the performance impact is negligible, we can combine these two ways together into only using the incremental way in the future. 


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


================
Comment at: lib/Transforms/Utils/Local.cpp:1062
+  // Zap all the instructions in the block.
+  while (!BB->empty()) {
+    Instruction &I = BB->back();
----------------
brzycki wrote:
> 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.
I understand why these zapping codes are needed. `DTU.applyUpdates()` do simple checks on whether the edge exists in the CFG. However, the `TerminatorInst` of `delBB` isn't removed, so the check just discard the update if `applyUpdates` is called before `deleteBB`. I'll modify this part of code into the logic of removing the `TerminatorInst` before calling `applyUpdates`.


Repository:
  rL LLVM

https://reviews.llvm.org/D48967