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

Brian Rzycki via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 14 08:44:11 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
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.

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list