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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:1108
+  if (PhisToCheck.size()) {
+    SmallVector<TrackingVH<MemoryPhi>, 16> PhisToOptimize;
+    for (MemoryPhi *MP : PhisToCheck)
----------------
george.burgess.iv wrote:
> Why is this a vector of `TrackingVH`?
> 
> I'm under the impression, though I haven't totally proven it to myself, that `onlySingleValue(MP)` implies that we'll never try to remove/RAUW/etc. `MP` in `tryRemoveTrivialPhi`, unless we're specifically calling `tryRemoveTrivialPhi` on `MP`. So, we shouldn't need to track anything here.
> 
> Happy to have this be an `AssertingVH` to assert that these nothing in this vector is dropped before we get to it, though.
Oh, I meant to remove the `onlySingleValue(MP)`.

If we're going to use `tryRemoveTrivialPhi`, which recurses internally, then it makes sense to actually let it remove more than a single `MemoryPhi`. Otherwise, we could just call `removeMemoryAccess`.

I meant to call `tryRemoveTrivialPhi` on the unfiltered list of `MemoryPhi`s, and have that list be `TrackingVH`, since some may go away (there's a test in that triggers this).

Updated now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57199