[PATCH] D58170: [DTU] Refine the logic of deduplication

Brian Rzycki via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 15 09:50:33 PST 2019


brzycki added inline comments.


================
Comment at: llvm/lib/Analysis/DomTreeUpdater.cpp:284
+        Seen.insert(Edge);
+        // If the update doesn't appear in the CFG, it means that
+        // either the change isn't made or relevant operations
----------------
brzycki wrote:
> NutshellySima wrote:
> > brzycki wrote:
> > > @NutshellySima it's probably me but I don't see how the comments match the code here. Let's take this example:
> > > ```
> > > Strategy: Lazy
> > > CFG: edge exists between A->B
> > > Updates: {{Delete, A, B}, {Insert, A, B}}
> > > U: {Delete, A, B}
> > > Edge: {A,B}
> > > isUpdateValid: true
> > > 
> > > ```
> > > In this case we will enqueue a deletion of edge A->B and not process the second update, the `{Insert, A, B}` because that `Edge` already exists in `Seen`.  If a `flush()` is called immediately after the state of `DTU` is incorrect.
> > > 
> > > Am I missing something here that prevents this case?
> > In the case you mentioned, `isUpdateValid: true` should be `isUpdateValid: false`.
> > 
> > `isUpdateValid` is a utility that inspects the current CFG and returns true when 
> > 1. `U = {Delete, A, B}` and there isn't any `A -> B edge` now.
> > 2. `U = {Insert, A, B}` and there is at least an `A -> B edge` now.
> > 
> > So, when we see the first update {Delete, A, B}, first, we can infer that the initial CFG which DTU is in sync with has the edge <A, B>; Second, by inspecting the current CFG via `isUpdateValid` which **returns ****`false`** in this case, we can know that there is still <A, B> now.
> > 
> > So, we can now reach a conclusion that 
> > 1. If there are further actions different from `Delete` on this edge, they all result in a no-op. Because when you submit the second update `{Insert, A, B}`, the precondition is that in the CFG produced by (the initial CFG + the first update `{Delete, A, B}`) [no matter whether the first one is a fake update], edge <A, B> doesn't exist. (Because you cannot `Insert` an existent edge.) In the case you mentioned, we can infer that the first update actually happened and the second update actually happened too.
> > 2. Otherwise (i.e., the whole Update array is made up of {Delete, A, B}s), they are all updates that never happened.
> Ah, got it! I was incorrectly reading `isUpdateValid()` while looking at the code yesterday.
> 
> I'm cautiously optimistic this will work. The real test will be the forthcoming patch to  change all the `*relaxed()` calls and see if we have asserts or breaks.
@NutshellySima it might be a good idea to add this reasoning to the comments of `applyUpdates()`. The interplay is subtle here and really needs to be documented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58170





More information about the llvm-commits mailing list