[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:172
+  assert(ClusterIdForPoint_.size() == Points_.size() && "size mismatch");
+  for (auto &&Point : zip(Points_, ClusterIdForPoint_)) {
+    const ClusterId &ClusterIdOfPoint = std::get<1>(Point);
----------------
courbet wrote:
> Why not `const auto&` ? 
Hm, i see all three variants within the codebase.
I guess `const auto&` is better.


================
Comment at: tools/llvm-exegesis/lib/Clustering.cpp:214
+      decltype(OldCluster.PointIndices) CleanedPointIndices;
+      // After removing 'tainted' points, we will drop at least/most one point.
+      CleanedPointIndices.reserve(OldCluster.PointIndices.size() - 1);
----------------
courbet wrote:
> at least, not at most.
Sure. I have meant that any other assumption would be too optimistic,
and we would end up reallocating if we guessed wrong.
A guess of one will never be too optimistic, and won't cause reallocations.
At worst, we will over-allocate a bit.


================
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:
> 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..


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58355