[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