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

[Chijun Sima via Phabricator via llvm-commits]

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


================
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,
----------------
kuhar wrote:
> I think this is fine with the eager strategy.
Considering updating DT/PDT under Eager mode is an idempotent action, I think it is applicable to delay the update validity scanning under Lazy mode to the time submitting updates to DT/PDT to avoid some fishy problems involved in JT before and also make applyUpdates() under Lazy mode also idempotent without using "point-based updates" which can have potential performance issues.


================
Comment at: llvm/include/llvm/Analysis/DomTreeUpdater.h:137
   /// CFG in DEBUG mode.
+  /// CAUTION! It is only legal to use this method when you have exactly one
+  /// insertion (and no deletions). This function has to be called *after*
----------------
mkazantsev wrote:
> That looks *highly* fishy. Between which two points we should have exactly one update?
> 
> For example, if we use eager strategy and we need to delete 10 edges and insert 10 other edges, is that guaranteed to work if we consecutively make 1 CFG update followed by 1 DT update, and do so for every edge? Can we interpret it as making exactly one update 20 times over?
> 
> Is that exactly one update between two flushes for lazy strategy?
When using DTU Eager UpdateStrategy or using DT/PDT directly, updating a DominatorTree doesn't follow associative law of conjunction because the internal updating logic of DomTree will do a graph search on the corresponding CFG each time you call the mutation APIs.
For example, 
```
// Don't make any change to CFG
// Considering Eager DomTree updates is idempotent
DT.insertEdge(Foo, Bar); // Assert here if there is no Foo->Bar in the original CFG
DT.deleteEdge(Foo, Bar); // Assert here if there is Foo->Bar in the original CFG
```
Considering `insertEdge/deleteEdge` can bring confusion to devs, I'll consider remove those 2 APIs but
```
// Don't make any change to CFG
// Considering Eager DomTree updates is idempotent
DT.applyUpdates({Insert,Foo, Bar}); // Assert here if there is no Foo->Bar in the original CFG
DT.applyUpdates({Delete,Foo, Bar}); // Assert here if there is Foo->Bar in the original CFG
```
is also not OK overall.


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

https://reviews.llvm.org/D57881