[PATCH] D57199: [MemorySSA] Extend removeMemoryAccess API to optimize MemoryPhis.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

> I would prefer to do the flag-flip separately though, in case it unearths some problematic usage.

SGTM

> There are already a few tests that cover this scenario: e.g., test/Transforms/EarlyCSE/memoryssa.ll.

Please note that in your commit message when this is landed, and I'm OK without a formal test-case here.



================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:1108
+  for (MemoryPhi *MP : PhisToOptimize)
+    if (onlySingleValue(MP))
+      removeMemoryAccess(MP, true);
----------------
asbirlea wrote:
> george.burgess.iv wrote:
> > It looks like this may result in a use-after-free. Consider a CFG:
> > 
> > ```
> > |---A
> > |  / \
> > | B   C
> > |/ \ /|
> > D<--E | 
> > \     /
> >  \   /
> >   \ / 
> >    F
> > ``` 
> > 
> > Control-flow flows down, except for the edge E -> D. (and there's an A->D edge that only looks somewhat awkward. :) )
> > 
> > All BBs are clear, though B has a Def in it. This Def would cause us to generate a Phi in E, D, and F.
> > 
> > So we remove that Def, remembering that the Phis in {D, E} are removal candidates. We check that E only has `liveOnEntry` first, it does, so we recurse on it. E's removal makes the Phi in D trivial, so we recurse on it, delete it, etc. We then land back at the top level of our stack and check `onlySingleValue(D)`.
> > 
> > If you buy all of this, ISTM we could either fix it by hoisting the `onlySingleValue` check into the `while (!MA->use_empty()) {` loop (if the Phi only points to one Def, we're not going to find it otherwise...). I'm willing to bet that the number of cases where the same Phi points to the same value N times is pretty small.
> > 
> > Alternatively, we could `takeVector()`, completely filter out the Phis that aren't `onlySingleValue`, from that, and *then* recurse on those Phis.
> Updated.
> Still calling removeMemoryAccess recursively.
> AFAICT we can reuse `tryRemoveTrivialPhi` and not pre-filter for `onlySingleValue`, but the vector will be `TrackingVH` since `tryRemoveTrivialPhi` will remove other phis in the vector being processed.
> Both approaches work. Let me know if you prefer to reuse `tryRemoveTrivialPhi`.
I'd prefer to centralize this logic as much as reasonable, so I like the `tryRemoveTrivialPhi` approach. (I forgot that existed -- thanks for catching that :) )

If `TrackingVH` doesn't work as set keys (I don't think it does, but am happy to be proven wrong), we can probably split the `SetVector` into `SmallVector<TrackingVH, ...>` and `SmallPtrSet<MemoryPhi *, ...>`


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57199