[PATCH] D72700: [DSE] Add first version of MemorySSA-backed DSE (Bottom up walk).

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1539
+  // Check if DomDef may be read.
+  for (unsigned I = 0; I < WorkList.size(); I++) {
+    MemoryAccess *UseAccess = WorkList[I];
----------------
asbirlea wrote:
> Walking uses may miss cases due to aliasing not being transitive. This needs to be throughly analyzed. Here's a very rough example.
> ```
> 1 = Def(LoE) ; <----- DomDef  stores [0,3]
> 2 = Def(1) ; (2, 1)=NoAlias,   stores [4,7]
> Use(2)      ; MayAlias 2 *and* 1, the Use points to the *first* Def it may alias, loads [0, 7].
> 3 = Def(1)  ; <---- Current  (3, 2)=NoAlias, (3,1)=MayAlias,  stores [0,3]
> ```
> The situation may be simplified due to handling stores, but all Uses may need looking at. Note this will not work to recurse on uses of the uses either.
> Rough example why:
> ```
> 1 = Def(LoE)
> 2 = Def(1)     ; <----- DomDef 
> 3 = Def(1) ; (3, 2)=NoAlias
> Use(3)      ; MayAlias 3 *and* 2, the Use points to the *first* Def it may alias.
> 4 = Def(2)  ; <---- Current  (4, 3)=NoAlias, (4,2)=MayAlias
> ```
> 
Thanks for the example. I think I might be missing something for the optimized version though. From the example in MemorySSA.h:

```
/// Given this form, all the stores that could ever effect the load at %8 can be
/// gotten by using the MemoryUse associated with it, and walking from use to
/// def until you hit the top of the function.
```

>From the comment above, shouldn't we visit both  2 and 3 when walking up from Use(3), as both may change the read location? In the example we would only see 3 and 1.

But even assuming we would visit both 2 and 3, I think I can see how we could end up with scenarios we could miss overlapping reads. I think we would have to take a look at all users of DomDef and we specifically *cannot* skip any non-aliasing MemoryDefs for the read-checks. 

That would make things more expensive, but would be something we have to do regardless of going bottom-up/top-down. Does that make sense to you? However I think that would mean that in the worst-case, we would have to do a top-down walk similar to the general top-down approach for the read checks.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1744
+      } else
+        SIMD = ND;
+    }
----------------
asbirlea wrote:
> I don't see how this being in a loop will work. Shouldn't this be a "give up"?
> Example:
> ```
> ; 1 = MemoryDef (LoE)
> store a
> ; 2 = MemoryDef(1)
> call_reading_a_and_overwriting_a
> ; 3 = MemoryDef(2)
> store a
> ```
> Once the getClobbering found a mayAlias of 3 with 2, even if an overwrite is not proven, def 1 cannot be removed.
> 
> I may have missed such a check in `getDomMemoryDef`. 
I had another look at the getClobberingMemoryAccess, and we indeed need an additional check ensuring that the memorydef does not also read the original location!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72700