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

[Tyker via Phabricator via llvm-commits]

Tyker added a comment.

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

thank you for the esplaination.



================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1599
+  // like object that doesn't escape.
+  if (SILocUnd && NonEscapingStackObjects.count(SILocUnd))
+    return false;
----------------
fhahn wrote:
> Tyker wrote:
> > 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.
> Do you have a particular test in mind? I could not come up with a test that fails, but I've added one that *should* check for the issue.
> 
> If we have reads of a stack object in a catch block, those would be visible in the MemorySSA and we would not find a suitable MemoryDef, right? Also, catchpad/landingpad instructions are marked as mayWriteToMemory, so they are currently MemoryDefs that alias everything (due to no having any info about the aliasing location).
my bad. after further checking this is correct


================
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:
> > `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
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72146