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

[Tyker via Phabricator via llvm-commits]

Tyker added a comment.

In D72146#1804134 <https://reviews.llvm.org/D72146#1804134>, @fhahn wrote:

> 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.


Ok seems reasonable, so we will change the core loop for each store will have to change in a following 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?

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 ?



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1599
+  // like object that doesn't escape.
+  if (SILocUnd && NonEscapingStackObjects.count(SILocUnd))
+    return false;
----------------
this condition seems to relaxed to me. i think we can skip barrier detection for stores to non-escaping pointers only if there are killed by an end of function, not when they are killed by a full overwrite.

if the store is killed by an overwriting store, an exception thrown between the 2 stores could be caught in the same function after the overwriting store. the store we are trying to kill would not be overwritten and it will be observable for the rest of the function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72146