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

[Florian Hahn via Phabricator via llvm-commits]

fhahn updated this revision to Diff 236258.
fhahn marked 2 inline comments as done.
fhahn added a comment.

Thank you very much for taking a look! I think I've addressed the comments. I also added a bunch of additional tests and comments.

In D72146#1804482 <https://reviews.llvm.org/D72146#1804482>, @Tyker wrote:

> [snip]
>
> > Yes as it is it does not cover that case. But I think it would be relatively straight forward to extend it to return a list of MemoryDefs encountered along multiple paths. Those can then be checked for legality separately. If some of the found Defs do not eliminate the original store, we can continue walking just those Defs.  To support that, it probably also makes sense to re-write getNextMemoryDef to work iteratively, like in your patch.
>
> Ok seems reasonable, so we will change the core loop for each store will have to change in a following patch.


+1

>> IMO the most productive way forward would be to get in a patch with limited but solid support as a baseline and once that is in we can work on adding missing functionality in parallel. What do you think about this approach?
> 
> you are right. i probably went a bit to far for a first patch.
> 
>> Besides extending the functionality, I think we will also have to spend a fair amount of work on making sure that preceding and succeeding passes in the pipeline preserve MemorySSA, to avoid computing it from scratch just for DSE.
> 
> yeah. and we may have to do the same thing for the post dominator tree.
>  is there a way to have a pass preserve an analysis if it is still valid without depending on it but not updating it if it is not valid ?

Yes, PostDominators is the second thing we need to get better at preserving. IIRC all places where we already use DomTreeUpdater we basically preserve PDT as well without any additional work, besides passing in the PDT to the updater. There's getAnalysisIfAvailable (legacy pass manager)/getCachedResult (new PM) to get an analysis, only if it is already computed. The only caveat is that the legacy pass manager cannot preserve function analysis if it is followed by a function pass (IIUC).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72146

Files:
  llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
  llvm/test/Transforms/DeadStoreElimination/MSSA/fence-todo.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/fence.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/memcpy-complete-overwrite.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/memcpy-lifetimes.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/memoryssa-scan-limit.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/memset-and-memcpy.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-captures.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-exceptions.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-loops.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-malloc-free.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-memintrinsics.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-memoryphis.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-simple.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/multiblock-throwing.ll
  llvm/test/Transforms/DeadStoreElimination/MSSA/simple.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D72146.236258.patch
Type: text/x-patch
Size: 91932 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200105/9fd7e395/attachment.bin>