[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 , comments are inline.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:420
   LoopHeaders.clear();
-  DDT->flush();
+  DTU->getDomTree();
   LVI->enableDT();
----------------
NutshellySima wrote:
> 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.
I'm concerned the complexity of updates will become unmanageable if you decide to take this route.  Some questions I have:
* How do you preserve the list of updates for one vs the other? Two separate queues?
* How do you decide to delete unnecessary updates from one and not the other?
* How do you manage the list of delted BBs? Are they only really deleted when both DT and PDT are flushed?
* How do you make sure you don't actually delete a BB early?
* How do you handle `Eager` vs `Lazy` updates?
* How do you handle `*Relaxed()` updates?

The longer you avoid flushing the further your updates become from the real CFG's shape. Debugging an assert where DT and PDT update at different times means the developer has to also carefully keep the state of DTU in mind instead of just the IR and the trees. I'm not sure if the possible saved overhead of updates to one or the other tree is worth all that added complexity.


================
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)));
----------------
NutshellySima wrote:
> 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. 
I always prefer simpler code if the performance impact is negligible. You'll be glad you did when you're furiously debugging a problem and an LLVM release window is coming up. :)


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:206
+  if (DTU) {
+    if (DTU->getUpdateStrategy() == DomTreeUpdater::UpdateStrategy::Eager) {
+      if (DTU->hasDomTree()) {
----------------
NutshellySima wrote:
> 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.
Sounds good, thanks!


================
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:
> > 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`.
Sounds like a cleaner solution, thank you for looking into this. I'll review the code when you update the change.


https://reviews.llvm.org/D48967