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

[Roman Lebedev via Phabricator via llvm-commits]

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");
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58355