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

[Chijun Sima via Phabricator via llvm-commits]

NutshellySima added a comment.

I find I have assumed some extra limitations:
Users need to **apply updates in the same order they made to CFG (at least for a same pair of <From, To>)**. 
Because the only functionality the Update array applied by users is inferring the existence of a specific edge in the initial CFG.



================
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 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58170