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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1472
+  if (auto CS = CallSite(DI))
+    if (CS.onlyAccessesInaccessibleMemory())
+      return true;
----------------
MemorySSA should handle this, if it doesn't we should fix that.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1536
+  };
+  PushMemUses(DomAccess);
+
----------------
Nit: Use only DomDef, known to be non-null outside the loop.
Restrict scope of DomAccess to inside the loop above.


================
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];
----------------
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
```



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1541
+    MemoryAccess *UseAccess = WorkList[I];
+    // Bail out on MemoryPhis for now.
+    if (isa<MemoryPhi>(UseAccess))
----------------
You're right this can be partially lifted.
Here's an example:
```
%a:
  1 = Def(LoE)  ; <----- DomDef 
   br %cond1, %b, %c
%b:
  2 = Def(1)
   br %cond2, %d, %e
%c:
   br %e
%d:
   br %f
%e:
   3 = phi(1,2)
   br %f
%f:
   4 = Phi(2,3)
   5 = Def(4) ; <---- Current
```
  


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1744
+      } else
+        SIMD = ND;
+    }
----------------
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`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72700