[PATCH] D48372: [MemorySSAUpdater] Remove deleted trivial Phis from active workset

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

I'm still in favor of just replacing `SmallVector<MemoryPhi *, 8> InsertedPHIs` with `SmallVector<WeakVH, 8> InsertedPHIs`, since:

- I think it meets all of our needs here,
- it's a very straightforward change (no need to deal with removal, new containers, etc.), and
- I don't see how we can ever get a duplicate in `InsertedPHIs` to start with.

In any case, a few questions/nits about the current patch.



================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:64
   MemorySSA *MSSA;
-  SmallVector<MemoryPhi *, 8> InsertedPHIs;
+  std::set<AssertingVH<MemoryPhi>> InsertedPHIs;
   SmallPtrSet<BasicBlock *, 8> VisitedBlocks;
----------------
Please add a comment saying that we're using a `std::set` because we need insertion to not invalidate iterators, since we mutate this set while iterating over it.

(I assume that's why we're using a set here, anyway)


================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:64
   MemorySSA *MSSA;
-  SmallVector<MemoryPhi *, 8> InsertedPHIs;
+  std::set<AssertingVH<MemoryPhi>> InsertedPHIs;
   SmallPtrSet<BasicBlock *, 8> VisitedBlocks;
----------------
george.burgess.iv wrote:
> Please add a comment saying that we're using a `std::set` because we need insertion to not invalidate iterators, since we mutate this set while iterating over it.
> 
> (I assume that's why we're using a set here, anyway)
Also, is it possible that this would cause us to visit Phis in a nondeterministic order (thus creating new Phis in a nondeterministic order, thus having unstable numbering for MemorySSA's print output)? If so, a straightforward fix would be to make a comparator that uses the Phi's `ID`.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:208
   if (Phi) {
+    InsertedPHIs.remove(Phi);
     Phi->replaceAllUsesWith(Same);
----------------
labrinea wrote:
> george.burgess.iv wrote:
> > george.burgess.iv wrote:
> > > labrinea wrote:
> > > > efriedma wrote:
> > > > > labrinea wrote:
> > > > > > efriedma wrote:
> > > > > > > remove on a SmallSetVector is linear time, which might be a problem here.
> > > > > > On a SmallVector we'd have to do a linear lookup and then move the elements around, so it should be the same in terms of complexity. Moreover, I thought it's good to have a guarantee that InsertedPHIs does not contain duplicates. What is the suggestion here?
> > > > > It's possible to construct a set with both constant-time removal and iteration in insertion order: essentially, make a `DenseMap<AssertingVH<MemoryPhi>, size_t>` where the value is an index into a `SmallVector<Optional<AssertingVH<MemoryPhi>>>`), or something like that.  Or maybe there's some simpler way to handle this I'm not thinking of.
> > > > I am not convinced it's worth maintaining two data structures to avoid a linear lookup, which costs nothing on a SmallVector. We do linear lookups for NonOptPhis.erase() anyway. I have a patch in case you are still interested but I am not in favour of it. In order for the DenseMap to be up to date we can't actually erase elements from InsertedPHIs but we just mark them as stale. This complicates other parts of the algorithm like insertDef() and fixupDefs().
> > > I'm surprised that we're removing anything here at all, given that we rely on stable indices in InsertedPhis on lines 299-305.
> > > 
> > > Would keeping a SmallVector<WeakVH, N> (with appropriate null checks/conversions where necessary) work just as well?
> > > 
> > > (I don't see how InsertedPHIs can get a duplicate, anyway. AFAICT, when we're inserting, either:
> > > 
> > > - The phi being inserted was just created, or
> > > - The phi being inserted was created earlier, but never had operands added, leaving it in a broken state. Prior to adding to InsertedPhis, we add operands, which should prevent us from adding it to InsertedPhis again
> > > 
> > > Am I missing something?)
> > Just bumping this comment, since I don't think that the 'stable indices' bit is addressed by the `OrderedSet` ADT, and don't see a clear path for addressing that (nor prose about why it's not a problem)
> fixupDefs() can potentially insert/remove MemoryPhis. As long as it only removes newly created Phis (not existing ones in the ADT), then the order of elements in the ADT shouldn't matter. Now looking at the line 304, it would make more sense if it was
> 
> ```
> FixupList.append(InsertedPHIs.begin() + StartingPHISize, InsertedPHIs.end());
> ```
> As long as it only removes newly created Phis (not existing ones in the ADT), then the order of elements in the ADT shouldn't matter

Sure, but are you 100% sure that it can't remove a phi that's not newly-created? :) I'd be willing to buy it, but haven't reasoned about this case enough to know for certain yet.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:297
 
-  while (!FixupList.empty()) {
-    unsigned StartingPHISize = InsertedPHIs.size();
-    fixupDefs(FixupList);
-    FixupList.clear();
-    // Put any new phis on the fixup list, and process them
-    FixupList.append(InsertedPHIs.end() - StartingPHISize, InsertedPHIs.end());
+  SmallPtrSet<const MemoryAccess *, 8> Seen;
+  unsigned cnt = 0;
----------------
Nit: If we only put Phis in here, please make it a set of `MemoryPhi`s, not `MemoryAccess`es.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:298
+  SmallPtrSet<const MemoryAccess *, 8> Seen;
+  unsigned cnt = 0;
+  while (cnt != InsertedPHIs.size()) {
----------------
Nit: `Cnt`


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:299
+  unsigned cnt = 0;
+  while (cnt != InsertedPHIs.size()) {
+    for (auto &MP : InsertedPHIs) {
----------------
In general, is there a "we'll never remove a MP from InsertedPhis if we visit it here" guarantee that I'm missing? If so, please add a comment about why we have that guarantee, since this loop appears to depend on it in a few ways.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:305
+        // We might have inserted/erased PHIs so we have to re-iterate.
+        cnt = 0;
+        break;
----------------
This is quadratic WRT the size of `InsertedPHIs`, no?


https://reviews.llvm.org/D48372