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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added inline comments.


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


https://reviews.llvm.org/D48372