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

Max Kazantsev via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Feb 12 02:59:16 PST 2019


mkazantsev added inline comments.


================
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*
----------------
NutshellySima wrote:
> 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.
I mean the following scenario:
  1. Delete A->B in control flow
  2.  DTU.deleteEdge(A, B)
  3. Delete B->C in control flow
  4. DTU.deleteEdge(B, C)
  ...

I can have two different interpretations of that:
a) It is not supposed to work under any strategy because I have more than 1 deletion.
b) It is only supposed to work under Eager strategy because at every point I make exactly one CFG update followed by DT update, and it is not supposed to work under Lazy strategy because I have more than 1 deletion.

>From this comment, I cannot figure out which interpretation is correct (if any).



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

https://reviews.llvm.org/D57881





More information about the llvm-commits mailing list