[llvm-exegesis] Move InstructionBenchmarkClustering::isNeighbour() into header

[lebedev.ri]

Summary

Old: (D54390)

 Performance counter stats for './bin/llvm-exegesis -mode=analysis -analysis-epsilon=100000 -benchmarks-file=/tmp/benchmarks.yaml -analysis-inconsistencies-output-file=/tmp/clusters.html' (10 runs):

       7432.421721      task-clock (msec)         #    1.000 CPUs utilized            ( +-  0.15% )
...
            7.4336 +- 0.0115 seconds time elapsed  ( +-  0.15% )

New:

 Performance counter stats for './bin/llvm-exegesis -mode=analysis -analysis-epsilon=100000 -benchmarks-file=/tmp/benchmarks.yaml -analysis-inconsistencies-output-file=/tmp/clusters.html' (10 runs):

       6569.936144      task-clock (msec)         #    1.000 CPUs utilized            ( +-  0.22% )
...
            6.5711 +- 0.0143 seconds time elapsed  ( +-  0.22% )

And another -12%. You'd think it would be inlined anyway, but no! :)

[MaskRay]

tools/llvm-exegesis/lib/Clustering.cpp

44

I think the emptiness check can get folded into isNeighbour

tools/llvm-exegesis/lib/Clustering.h

92

It is interesting that it does not get inlined... Does an inlinespecifier help?

isNeighbour is also used in tools/llvm-exegesis/lib/Analysis.cpp#L503. I'm not familiar with the code but I guess there the performance may not matter too much.

[lebedev.ri]

tools/llvm-exegesis/lib/Clustering.cpp

44

If this check is here it says "there are no measurements, therefore we can't determine
whether it is a neighbor or not".
If it is in isNeighbour(), it will say "we do not have any measurements for that point,
so let's assume that it is located more than EpsilonSquared_ from the current point,
and is not a neighbour".

As per c++ syntax, these will be equivalent.
But will that not conceal the actual algo? I'm not sure.

tools/llvm-exegesis/lib/Clustering.h

92

That won't (doesn't) work.
If i mark either both the declaration and definition, or either one of them, with inline,
linking fails with this symbol being missing.

This function is called multiple times from the innermost loops.
It *should* be in the header.

[courbet]

tools/llvm-exegesis/lib/Clustering.cpp

44

But will that not conceal the actual algo? I'm not sure.

Let's keep it like this for clarity.

tools/llvm-exegesis/lib/Clustering.h

92

moving to the class declaration makes the function implicitly inline (https://en.cppreference.com/w/cpp/language/inline, second sentence), so you don't need the inline here.

[lebedev.ri]

@MaskRay @courbet thank you for the reviews!

tools/llvm-exegesis/lib/Clustering.h

92

Good point. Will adjust.

tools/llvm-exegesis/lib/Clustering.h

if (Id.isError()) {
       return ErrorCluster_;
     }
     return Clusters_[Id.getId()];
   }
 
   const std::vector<Cluster> &getValidClusters() const { return Clusters_; }
 
   // Returns true if the given point is within a distance Epsilon of each other.
-  bool isNeighbour(const std::vector<BenchmarkMeasure> &P,
-                   const std::vector<BenchmarkMeasure> &Q) const;
+  inline bool isNeighbour(const std::vector<BenchmarkMeasure> &P,
+                          const std::vector<BenchmarkMeasure> &Q) const {
+    double DistanceSquared = 0.0;
+    for (size_t I = 0, E = P.size(); I < E; ++I) {
+      const auto Diff = P[I].PerInstructionValue - Q[I].PerInstructionValue;
+      DistanceSquared += Diff * Diff;
+    }
+    return DistanceSquared <= EpsilonSquared_;
+  }
 
 private:
   InstructionBenchmarkClustering(
       const std::vector<InstructionBenchmark> &Points, double EpsilonSquared);
   llvm::Error validateAndSetup();
   void dbScan(size_t MinPts);
   void rangeQuery(size_t Q, llvm::SmallVectorImpl<size_t> &Scratchpad) const;
 

tools/llvm-exegesis/lib/Clustering.cpp

 void InstructionBenchmarkClustering::rangeQuery(
     const size_t Q, llvm::SmallVectorImpl<size_t> &Neighbors) const {
   Neighbors.clear();
   const auto &QMeasurements = Points_[Q].Measurements;
   for (size_t P = 0, NumPoints = Points_.size(); P < NumPoints; ++P) {
     if (P == Q)
       continue;
     const auto &PMeasurements = Points_[P].Measurements;
     if (PMeasurements.empty()) // Error point.
       continue;
     if (isNeighbour(PMeasurements, QMeasurements)) {
       Neighbors.push_back(P);
     }
   }
 }
 
-bool InstructionBenchmarkClustering::isNeighbour(
-    const std::vector<BenchmarkMeasure> &P,
-    const std::vector<BenchmarkMeasure> &Q) const {
-  double DistanceSquared = 0.0;
-  for (size_t I = 0, E = P.size(); I < E; ++I) {
-    const auto Diff = P[I].PerInstructionValue - Q[I].PerInstructionValue;
-    DistanceSquared += Diff * Diff;
-  }
-  return DistanceSquared <= EpsilonSquared_;
-}
-
 InstructionBenchmarkClustering::InstructionBenchmarkClustering(
     const std::vector<InstructionBenchmark> &Points,
     const double EpsilonSquared)
     : Points_(Points), EpsilonSquared_(EpsilonSquared),
       NoiseCluster_(ClusterId::noise()), ErrorCluster_(ClusterId::error()) {}
 
 llvm::Error InstructionBenchmarkClustering::validateAndSetup() {
   ClusterIdForPoint_.resize(Points_.size());