[PATCH] D72146: [DSE] Add first version of MemorySSA-backed DSE.

Tue Jan 14 06:36:00 PST 2020

fhahn added a comment.

In D72146#1818425 <https://reviews.llvm.org/D72146#1818425>, @asbirlea wrote:

> Some comments inline.
>
> Re-adding the inline comment: downwards walks are inefficient. This also currently misses optimization opportunities. In order to not miss anything, all Defs need to be looked at, not only uses, adding more overhead. So it's possible replacing MemDepAnalysis with MemorySSA may not get many benefits. 
>  Maybe do some preliminary performance testing to check trade-offs between this recursive approach, the iterative approach in Tyker's patch, or consider a bottom-up approach?

Thanks for taking a look! I've experimented with a bottom up walk, which tries to find MemoryDefs that may be killed by a given MemoryDef: D72700 <https://reviews.llvm.org/D72700>. Roughly, the approach uses getClobberingAccess to find dominating MemoryDefs that alias the original location and may be killed by the starting def. In order to kill a def we found, we also have to make sure that there are no clobbering reads between the 2 defs. I think for that, we have to look at the users of the dominating def and check if any of them are read clobbers. I *think* we still end up with needing to check fewer uses. Does the bottom-up approach make sense to you?

I still have to think a bit more about a few points below (I would suggest adding them as follow-up patches anyways), but it would be great to hear your thoughts about using the bottom-up approach over top-down, before addressing the remaining comments here.

1. deal with PHIs both when walking up and also when checking the users.
2. how to support eliminating stores like

     store i32 0, i32* %Ptr.  ; DEAD
     br i1 %cond, label %a, label %b
  a:
    store i32 2, i32* %Ptr
     br label %c
  b: 
    store i32 2, i32* %Ptr
    br label %c

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72146