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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Regarding handling the branching example, the solution fhahn proposes sounds reasonable to me.
I was thinking something similar, along the lines of: do the checks bottom-up from both second and third stores during the main algorithm, both find the dead store but do not postdominate it; don't abandon them, but keep this info (include the checks for no intervening reads) for after the main search. The data structure could be a Hashmap<PotentialDeadStore, ListOfDefsOrBlocksWhoFoundThisPotentialDeadStore>. If all paths are covered in the List for a PotentialDeadStore, then it's truly dead.



================
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];
----------------
fhahn wrote:
> 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.
I think I know what I missed. MemoryDefs keep two fields, the defining and the optimized access. So `3 = Def(1) ` actually looks like this: `3 = Def(2) - Optimized 1 `, and is a user of both 1 and 2.

Yes, I think you're right that the read checks for all accesses are needed regardless of which approach is taken. And yes, it will be expensive. 
I came across something similar in LICM, and I limited or outright avoided analyzing all uses against a store to avoid the cost of analyzing all of them (see ~LICM.cpp:1300)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72700