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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

> It also adds and additional overhead

Any targeted fix that makes us track the lifetime of these more closely is going to add additional overhead. If anything, I imagine that adding a DenseMap insertion/deletion per Phi would be cheaper than taking n^2 time to walk said Phis.

> In general, the idea of keeping null references in the ADT instead of actually deleting them seems like a workaround to me. I don't see why we should be afraid of making non trivial changes in the code if there's a fundamental bug.

I think the fundamental issue is that we create these non-minimal phi structures just to zap them shortly afterward. :) From the looks of this, though, that's a limitation imposed by the algorithm we're using. So, unless we want to try and improve that algorithm, or unless our goal is to swap to another algorithm without this limitation, anything we do here is going to boil down to a "workaround" of that quirk.

A Phi is being deleted and we need later iterations of this algorithm to take that into account. I don't understand why eagerly removing a pointer from our worklist is strictly less hacky than using `WeakVH` for this purpose.

If you still feel strongly that `WeakVH` isn't the right way, I'm happy to accept an eager removal approach if it doesn't substantially lose out in overall clarity, simplicity, and (algorithmic, within reason) performance over just using `WeakVH`. I don't think this patch is at that point yet, and I think we'd need a custom SetVector-but-with-constant-time-bounds, as was proposed, to get it there. (I also suspect that such a patch would end up losing out in perf/memory over `WeakVH` in the constant factor, but I'm sure we have lower hanging fruits than that in this code, if the speed/memory consumption here are becoming problematic.)

> I don't see why we should be afraid of making non trivial changes in the code

I'm fine with making substantial changes to code if doing so is called for. My concern is more that it's not called for here. :)

> On that note I still believe the line 303 of the original revision is wrong

Agreed



================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:64
   MemorySSA *MSSA;
-  SmallVector<MemoryPhi *, 8> InsertedPHIs;
+  std::set<AssertingVH<MemoryPhi>> InsertedPHIs;
   SmallPtrSet<BasicBlock *, 8> VisitedBlocks;
----------------
labrinea wrote:
> george.burgess.iv wrote:
> > 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`.
> > 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)
> 
> We use std::set is to avoid linear time removal from SmallVector, a concern raised by @efriedma. This was reason I created https://reviews.llvm.org/D49030. However, people from the community were tentative about adding a new ADT that is very similar to existing ones (MapVector,SetVector), which are not a good fit for InsertedPHIs though.
> 
> > 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
> 
> Sounds like a good idea.
> We use std::set is to avoid linear time removal from SmallVector

Please prefer `DenseSet` (or one of its variants) if all that's needed is fast set-like operations. `std::set` provides a number of guarantees above and beyond `DenseSet` (iterator validity, pointer stability, ...), and so generally ends up being slower.

That said, if we go with using IDs as keys, we'll need the ordering guarantee that `std::set` provides, so please add a comment noting that we're using a set for consistent Phi ordering.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:103
           Phi->addIncoming(PhiOps[i++], Pred);
-        InsertedPHIs.push_back(Phi);
+        InsertedPHIs.insert(Phi);
       }
----------------
Please also `assert` that this insertion happened, so it's clear that deduplication is never a goal here.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:299
+  unsigned cnt = 0;
+  while (cnt != InsertedPHIs.size()) {
+    for (auto &MP : InsertedPHIs) {
----------------
labrinea wrote:
> george.burgess.iv wrote:
> > 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.
> No we don't have such a guarantee that's why we have to break the innermost loop and re-iterate. 
I don't think that works in the general case, since our `Seen` set will retain dangling pointers. If we delete a Phi and `new` up one at the same pointer, we'll fail to visit it here. It's admittedly a rare case, but certainly a possibility.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:305
+        // We might have inserted/erased PHIs so we have to re-iterate.
+        cnt = 0;
+        break;
----------------
labrinea wrote:
> george.burgess.iv wrote:
> > This is quadratic WRT the size of `InsertedPHIs`, no?
> Possibly.
With the std::set comparator change, I'd say "definitely". :)


https://reviews.llvm.org/D48372