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

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added inline comments.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:208
   if (Phi) {
+    InsertedPHIs.remove(Phi);
     Phi->replaceAllUsesWith(Same);
----------------
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().


https://reviews.llvm.org/D48372