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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn 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);
----------------
Tyker wrote:
> 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.
I think for now it's probably better to avoid adding them to MemorySSA. I've re-added the capture check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72146