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

[Tyker via Phabricator via llvm-commits]

Tyker added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1678
+    // then it's leaked by this function anyways.
+    else if (isAllocLikeFn(&I, &TLI) && !PointerMayBeCaptured(&I, false, true))
+      NonEscapingStackObjects.insert(&I);
----------------
fhahn wrote:
> Tyker wrote:
> > fhahn wrote:
> > > Tyker wrote:
> > > > `PointerMayBeCaptured(&I, false, true)`
> > > > i think we should care about return capture here. if the pointer is returned the content is observable.
> > > As long as we do not use it for eliminating stores due to reaching the end of the walk, it should be fine to ignore return captures. I think all we care about here is that they are not known to the caller, so it is safe to eliminate stores even if we encounter a function that may throw.
> > > 
> > > After thinking a bit more, I think we do not need to check for capturing here at all. Function calls are modeled in MemorySSA and we currently treat them as aliasing everything. So if a pointer escapes to such a function, we would catch that through the MemorySSA.
> > > 
> > > I've renamed NonEscpaingStackObjects to make it a bit clearer and also added more explanation in comments.
> > ok. in the with the current uses not checking the return seems correct.
> > 
> > but not checking at all for capture doesn't seem correct.
> > capturing function calls can be anywhere in the function not necessarily between the 2 stores so they may not be reached by a MemorySSA traversal. and the throw may be attributed such that it doesn't appear in MemorySSA.
> > removing `store i8 2, i8* %call, align 1` is not legal in the following example.
> > 
> > ```
> > define void @f() {
> >   %call = call i8* @malloc(i32 1)
> >   call void @may_capture(i8* %call)
> >   store i8 2, i8* %call, align 1
> >   call void @may_throw()
> >   store i8 3, i8* %call, align 1
> >   ret void
> > }
> > 
> > declare i8* @malloc(i32)
> > declare void @may_capture(i8*)
> > declare void @may_throw() readnone
> > ```
> Ah right, I think it basically boils down to whether readnone functions can throw or not. I think from the definition in the langref, for C/C++, they cannot throw exceptions (or modify any state visible to the caller). 
> 
> But you are right, after another reading I realized that it states that there may be other mechanisms that may throw without modifying visible state in other languages. I'll bring back the check. I wonder if there's a way to allow skipping the checks for C/C++.
If all throwing instruction where in MemorySSA. We wouldn't need any checking of throwing instruction outside of the MemorySSA traversal this would simplify and most likely speedup the pass.

I think that changing MemorySSA such that throwing instruction are in MemorySSA even if they don't access memory is probably the best solution. but we need the opinion of a maintainer of MemorySSA for this.
An other solution is just to check for throwing instruction not present in MemorySSA before proceeding with the function and if there is any fallback to what I did in my patch and check it separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72146