[PATCH] D57881: [DTU] Refine the document of mutation APIs [NFC] (PR40528)

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

Nice, thanks for working on this!

As a general direction, I think we should try to minimize the amount of documentation about DT updates -- the number of different ways and all their preconditions are confusing.

I'd like to first go forward with this patch, after minor corrections, and then mark insertEdge/deleteEdge as deprecated, migrate their uses to applyUpdates, and later remove it entirely. The, we can note put a note in GenericDomTree saying that for the llvm trees,  DTU should be used instead.



================
Comment at: llvm/include/llvm/Analysis/DomTreeUpdater.h:93
+  /// 1. via the DomTreeUpdater class:
+  /// a. Eager UpdateStrategy: It is a bare wrapper of the incremental updater
+  /// of the DominatorTree class.
----------------
I'd add some indentation.


================
Comment at: llvm/include/llvm/Analysis/DomTreeUpdater.h:94
+  /// a. Eager UpdateStrategy: It is a bare wrapper of the incremental updater
+  /// of the DominatorTree class.
+  /// b. Lazy UpdateStrategy: It is recommended to use this update strategy
----------------
Instead of referring to the DT incremental updater, I'd say that it performs updates immediately.


================
Comment at: llvm/include/llvm/Analysis/DomTreeUpdater.h:101
+  /// process depending on the number of updates.
+  /// 2. via the DominatorTree class:
+  /// a. incremental updater APIs in the DominatorTree class: It is always *not*
----------------
Instead of listing this as another method, I'd mention that even though GenericDomTree provides these update primitives, it is recommended to use DTU with llvm::DomTree and llvm::PostdomTree. I think the less text here the better.


================
Comment at: llvm/include/llvm/Analysis/DomTreeUpdater.h:106
+  /// transformations.
+  /// b. low-level APIs in the DominatorTree class: These APIs are more
+  /// error-prone but is typically faster than the incremental updater when you
----------------
I'd completely omit the mention of the 'manual' update API -- there are very few legitimate reasons to use it now 


================
Comment at: llvm/include/llvm/Analysis/DomTreeUpdater.h:127
+  /// 2. It is illegal to submit any (From, To) pair that the DomTreeUpdater
+  /// or the DominatorTree already acknowledged.
   void applyUpdates(ArrayRef<DominatorTree::UpdateType> Updates,
----------------
I think this is fine with the eager strategy.


================
Comment at: llvm/include/llvm/Analysis/DomTreeUpdater.h:141
+  /// update that the DomTreeUpdater or the DominatorTree already acknowledged.
   void insertEdge(BasicBlock *From, BasicBlock *To);
 
----------------
> update that the DomTreeUpdater or the DominatorTree already acknowledged.
update that has already been applied.



================
Comment at: llvm/include/llvm/Analysis/DomTreeUpdater.h:153
+  /// insertion (and no deletions). It is illegal to submit any
+  /// update that the DomTreeUpdater or the DominatorTree already acknowledged.
   void insertEdgeRelaxed(BasicBlock *From, BasicBlock *To);
----------------
like above


================
Comment at: llvm/include/llvm/Analysis/DomTreeUpdater.h:207
+  /// awaiting deletion.
+  /// Does nothing under Eager UpdateStrategy.
+  void flush();
----------------
under the


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57881/new/

https://reviews.llvm.org/D57881