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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: tools/llvm-exegesis/lib/Clustering.cpp:44
     const auto &PMeasurements = Points_[P].Measurements;
     if (PMeasurements.empty()) // Error point.
       continue;
----------------
MaskRay wrote:
> I think the emptiness check can get folded into `isNeighbour`
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.


================
Comment at: tools/llvm-exegesis/lib/Clustering.h:92
   // 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 {
----------------
MaskRay wrote:
> It is interesting that it does not get inlined... Does an `inline`specifier 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D54393