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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1599
+  // like object that doesn't escape.
+  if (SILocUnd && NonEscapingStackObjects.count(SILocUnd))
+    return false;
----------------
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).


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72146