[PATCH] D48383: [Dominators] Add the DomTreeUpdater class

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

Thank you for working on this @NutshellySima , a unified dominator interface will be most welcome in LLVM. Please see my inline comments.



================
Comment at: include/llvm/IR/DomTreeUpdater.h:116
+  /// This function should be called when the class holds a PostDomTree.
+  void recalculatePostDomTree(Function &F);
+  /// Recalculate Both of the trees.
----------------
NutshellySima wrote:
> kuhar wrote:
> > Similar to above.
> > One more thing: why do we expose these function? Is it possible that someone wants to recalculate just one tree and leave the other one intact? If this is the case, can you explain when it would make sense?
> As mentioned in the DeferredDomiance document, the DT traversal algorithm needs all BasicBlocks awaiting deletion be deleted before it determines the actual DT. So there should be such recalculate* functions to determine whether to leave the other tree intact (check whether there are BasicBlocks awaiting deletion).
You might want to add a comment explaining why deleted BBs need to be processed first:
```
/// If there are BasicBlocks pending deletion they must be removed first.
/// This is necessary because the pending BBs still have their Parent set to F
/// which is traversed by the tree's recalculate() algorithm and will assert()
/// if it detects a block in F that is not dominated by entry.


================
Comment at: include/llvm/IR/DomTreeUpdater.h:119
+  /// This function should be called when the class holds both of the trees.
+  void recalculateAll(Function &F);
+  // flush
----------------
kuhar wrote:
> Why doesn't it recalculate whatever trees you have?
I strongly dislike the idea of recalculation of DT or PDT by the caller. If the user has instantiated a DTU with DT and PDT I'd expect the class to quietly operate (and update) both. I cannot think of a situation where the caller would want DT updated but PDT not updated.


================
Comment at: include/llvm/IR/DomTreeUpdater.h:170
+  /// \returns false when nothing deleted.
+  /// \returns true otherwise.
+  bool forceFlushDeletedBB();
----------------
kuhar wrote:
> maybe: `\returns true is at least one BB was deleted`?
+1


================
Comment at: include/llvm/IR/DomTreeUpdater.h:70
+  /// the updates will be queued for update. It is required for the the state of
+  /// the LLVM IR to be applied *before* submit the Updates for the internal
+  /// update routine will analyze the current state of the relationship between
----------------
The comment here doesn't make grammatical sense. Did you mean "It is required for the state of the LLVM IR to be updated *before* applying updates to the DominatorTree. This routine inspects the current IR edge relationships to determine if an update needs to be queued."


================
Comment at: include/llvm/IR/DomTreeUpdater.h:82
+  /// Lazy Strategy, an update not applied to the LLVM IR before will be
+  /// silently discarded. This is best used when there is only a single
+  /// insertion needed to update Dominators.
----------------
I find the wording of this comment to be a bit confusing. I understand it's difficult to describe all the state you're trying to convey but maybe it's better to rewrite a bit. Here's more along the lines of what I was thinking:
```
/// Notify all holding trees on an edge insertion.
/// The internal Strategy of the DTU instance alters how the insertion even is handled.
///The Eager Strategy applies the update immediately while the Lazy Strategy will
/// queue them until a flush() event. If the current IR does not contain the edge
/// inserted the update will be silently discarded unless the user sets
/// ForceRemoveInvalidUpdate to true. It is recommended to only use this method
/// when you have exactly one insertion (and no deleteions) needed to the DTU.
/// It is recommended to use applyUpdates() in all other cases.
```


================
Comment at: include/llvm/IR/DomTreeUpdater.h:87
+
+  /// Notify all holding trees on an edge deletion.
+  /// Under Eager Strategy and ForceRemoveInvalidUpdate equals to false,
----------------
Comments similar to insertEdge()...


================
Comment at: include/llvm/IR/DomTreeUpdater.h:142
+  /// It must only be called when it has a DomTree.
+  DominatorTree &flushDomTree();
+
----------------
I'd actually prefer if flushDomTree() and flushPostDomTree(), and flushAll() were private members. Users shouldn't have to think about flushing state. Instead, have all delections and flushes happen when they request access to a DT or PDT reference such as:

```
auto DTU = DomTreeUpdater(DT, PDT, Lazy);
...
DTU->applyUpdates({Foo, Bar, Insert});
DTU->applyUpdates({Bar, Baz, Delete});
...
auto DT = DTU->getDT(); // flush() and delete BBs happen here automatically.
```


================
Comment at: include/llvm/IR/DomTreeUpdater.h:182
+  /// Remove all the instrcutions of DelBB and make sure DelBB has a valid
+  /// terminator instruction. Assert if DelBB is nullptr or has predecessors.
+  void validateDeleteBB(BasicBlock *DelBB);
----------------
It'd be good to mention why this is necessary in the comment. When immediately deleting a BB, the call `removeFromParent()` removes all its instructions as well. However, in the deferred case the block isn't really deleted and is still a child of F. This causes problems for algorithms that do `for (auto BB: F)` and expect terminator instructions, instruction Uses, and REPL to all be consistent for every block inside a function.  Without these fixups all sorts of routines will start asserting() complaining that the state of the IR is inconsistent.


https://reviews.llvm.org/D48383