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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

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

> In D72146#1803859 <https://reviews.llvm.org/D72146#1803859>, @fhahn wrote:
>
> > Do you think it would make sense to use D72146 <https://reviews.llvm.org/D72146>  (this patch) as foundation and port the improvements from your patch? It would be great if you could have a look and let me know what you think!
>
>
> I took a look at this patch and D72148 <https://reviews.llvm.org/D72148>(the patch adding MemPhi traversal). they have many good ideas i haven't thought about.
>  but i have one major concern.
>  how to you plan to dealing with store being killed by multiple stores which together post-dominate the "dead" store ?
>  like the store in the entry block of the following:
>
>   define void @test5A(i32* %P) {
>     store i32 1, i32* %P ; this is dead
>     br i1 true, label %bb1, label %bb2
>   bb1:
>     store i32 0, i32* %P
>     br label %bb3
>   bb2:
>     store i32 1, i32* %P
>     br label %bb3
>   bb3:
>     ret void
>   }
>
>
> the design around `getNextMemoryDef` seems to me like it is too restrictive for the above example.


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.

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?

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.

> aside from that we went for different strategies for storing pass related data structures but i am not locked on my decision of having a `DSEContext` storing "everything".




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72146