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

[Clement Courbet via Phabricator via llvm-commits]

courbet accepted this revision.
courbet added inline comments.


================
Comment at: tools/llvm-exegesis/lib/Clustering.cpp:44
     const auto &PMeasurements = Points_[P].Measurements;
     if (PMeasurements.empty()) // Error point.
       continue;
----------------
lebedev.ri wrote:
> 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.
> But will that not conceal the actual algo? I'm not sure.

Let's keep it like this for clarity.


================
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 {
----------------
lebedev.ri wrote:
> 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D54393