[PATCH] D54007: Use a data structure better suited for large sets in SimplificationTracker.

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope requested changes to this revision.
bjope added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2713
+      NodeSet.erase(Ptr);
+      SkipRemovedElements(FirstValidElement);
+      return true;
----------------
Imagine that we insert the value 1,2,3,4,5 into the set, then the NodeList will be `[1, 2, 3, 4, 5]`.
Now we remove "3", then the NodeList is untouched and FirstValidElement still points at the beginning of the NodeList.
If we later inset "3" again, then  the NodeList will become `[1, 2, 3, 4, 5, 3]`.
When iterating we now will get the order 1,2,3,4,5 and not the expected 1,2,4,5,3 (assuming that we really expect to get the insertion order).

I'm not sure if the above will ever happen for the specific use case in CodeGenPrepare, but looking at this as a general set implementation it seems faulty.

One solution, seeing this as a set of pointers, could be to now allow nullptr in the set (so add an assert for the in "insert"). And then we should not only move FirstValidElement forward here, we also need to scan the NodeList starting at FirstValidElement and replace any found entry matching `Ptr` with a nullptr (there should be exactly one such entry since it is a unique set).


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2790
   const SimplifyQuery &SQ;
-  // Tracks newly created Phi nodes. We use a SetVector to get deterministic
-  // order when iterating over the set in MatchPhiSet.
-  SmallSetVector<PHINode *, 32> AllPhiNodes;
+  // Tracks newly created Phi nodes. The elements will iterated by insertion
+  // order.
----------------
nit: I think it should say "elements are iterated" or "elements will be iterated"


Repository:
  rL LLVM

https://reviews.llvm.org/D54007