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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

@MaskRay @courbet thank you for the reviews!



================
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 {
----------------
courbet wrote:
> 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.
Good point. Will adjust.


Repository:
  rL LLVM

https://reviews.llvm.org/D54393