[PATCH] D58355: [llvm-exegesis] Opcode stabilization / reclusterization (PR40715)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 2 inline comments as done.
lebedev.ri added inline comments.


================
Comment at: tools/llvm-exegesis/lib/Clustering.cpp:216
+      CleanedPointIndices.reserve(OldCluster.PointIndices.size() - 1);
+      for (size_t P : OldCluster.PointIndices) {
+        assert(ClusterIdForPoint_[P] == OldCluster.Id && "sanity check");
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > courbet wrote:
> > > lebedev.ri wrote:
> > > > courbet wrote:
> > > > > This for loop + CleanedPointIndices could be removed using `std::remove_if`:
> > > > > 
> > > > > ```
> > > > > // Find which points should be moved to the new cluster.
> > > > > const auto it = std::remove_if(OldCluster.PointIndices.begin(), 
> > > > >                               OldCluster.PointIndices.end(),
> > > > >                               [this, UnstableOpcode](size_t P){ return Points_[P].keyInstruction().getOpcode() == UnstableOpcode; });
> > > > > // Move removed points to the new cluster:
> > > > > UnstableCluster.PointIndices.insert(it, OldCluster.PointIndices.end());
> > > > > // Remove points form the old cluster.
> > > > > OldCluster.PointIndices.erase(it, OldCluster.PointIndices.end());
> > > > > ```
> > > > > 
> > > > > 
> > > > Hmm, are you sure?
> > > > 
> > > > http://cpp.sh/7dfh5
> > > > If that is so, then that ^ should have
> > > > printed `Textwithsomewhitespaces       ` but it prints `Textwithsomewhitespacesespaces`.
> > > > 
> > > > https://en.cppreference.com/w/cpp/algorithm/remove
> > > > ```
> > > > Iterators pointing to an element between the new logical end and the physical end of the range are still dereferenceable, but the elements themselves have unspecified values (as per MoveAssignable post-condition). 
> > > > ```
> > > > `unspecified values` is pretty self-explanatory..
> > > Yes, sorry. `std::stable_partition` should work.
> > Aha. Not as simple as that snippet, but works.
> Thinking about this a bit more, do we really care that "Relative order of the elements is preserved."?
> I don't think so. Only that the new order is deterministic.
> Can we just use [[ https://en.cppreference.com/w/cpp/algorithm/partition | `std::partition` ]] instead?
Actually, hmm, i'm not confident `std::partition` *is* deterministic.
Never mind.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58355/new/

https://reviews.llvm.org/D58355