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

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: tools/llvm-exegesis/lib/Clustering.cpp:168
+  OpcodeToClusterIDs.resize(NumOpcodes);
+  // Okay, so which Opcodes have more than one cluster?
+  llvm::SetVector<size_t> UnstableOpcodes;
----------------
"The list of opcodes that have more than one cluster".


================
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);
----------------
Why not `const auto&` ? 


================
Comment at: tools/llvm-exegesis/lib/Clustering.cpp:192
+  Clusters_.reserve(NewTotalClusterCount);
+  for (size_t UnstableOpcode : UnstableOpcodes.getArrayRef()) {
+    const llvm::SmallSet<ClusterId, 1> &ClusterIDs =
----------------
`for (const size_t UnstableOpcode` for safety.


================
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);
----------------
at least, not at most.


================
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");
----------------
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());
```




================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:444
 
-  const Analysis Analyzer(*TheTarget, Clustering);
+  std::unique_ptr<llvm::MCInstrInfo> InstrInfo_;
+  InstrInfo_.reset(TheTarget->createMCInstrInfo());
----------------
nit: `InstrInfo`


================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:445
+  std::unique_ptr<llvm::MCInstrInfo> InstrInfo_;
+  InstrInfo_.reset(TheTarget->createMCInstrInfo());
+
----------------
Why not: `std::unique_ptr<llvm::MCInstrInfo> InstrInfo(TheTarget->createMCInstrInfo());` ?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58355