[PATCH] D81050: [llvm-exegesis] Let Counter returns up to 16 entries.

[Ondrej Sykora via Phabricator via llvm-commits]

ondrasej added a comment.

I've added a couple of style comment + one bigger comment on the aggregation of results from multiple runs/counter buffer.



================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:56
+  static void
+  accumulateCounterValues(const llvm::SmallVector<int64_t, 4> NewValues,
+                          llvm::SmallVector<int64_t, 4> *Result) {
----------------
NewValues should be a const reference, no?


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:65
+      ++i;
+    }
+  }
----------------
This is probably not a big deal, but consider:
```
const size_t NumValues = std::max(NewValues.size(), Result->size());
if (NumValues > Result->size()) Result->resize(NumValues, 0);
for (size_t i = 0, End = NewValues.size(); i < End; ++i) {
  (*Result)[i] += NewValues[i];
}
```
for unrolled + vectorized code.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:73
+    llvm::SmallVector<int64_t, 4> CounterValues;
+    int reserved = 0;
     SmallVector<StringRef, 2> CounterNames;
----------------
s/reserved/Reserved/


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:87
+      if (reserved == 0)
+        CounterValues.reserve(reserved = Counter->numValues());
+      else if (reserved != Counter->numValues())
----------------
I'd prefer not using assignment in function arguments - this might be easily overlooked or mistaken for a bug (assignment in place of equality comparison).


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:41
+  for (const auto &V : Values)
+    Ret += std::pow(V - Mean, 2);
+
----------------
std::pow is somewhat heavy-weight as it is not optimized for integer exponents,
const double Delta = V - Mean;
Ret += Delta * Delta;
will likely be significantly faster.


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:53
+      Min = V;
+  return Min;
+}
----------------
A shorter way to write this:
```
return std::accumulate(
  Values.begin(), Values.end(), std::numeric_limits<int64_t>::max(),
  [](int64_t A, int64_t B) { return A < B ? A : B; });
```
And similar for FindMax below.


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:81
+    double CurVariance = ComputeVariance(*ExpectedCounterValues);
+    if (Variance > CurVariance)
+      WithMinVariance = *ExpectedCounterValues;
----------------
Nit: Since we have WithMinVariance, consider renaming Variance to MinVariance, and CurVariance to just Variance.


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:82
+    if (Variance > CurVariance)
+      WithMinVariance = *ExpectedCounterValues;
+    Variance = CurVariance;
----------------
Ideally, this should use move semantics (if they are supported by SmallVector).


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:83
+      WithMinVariance = *ExpectedCounterValues;
+    Variance = CurVariance;
   }
----------------
This would update Variance (MinVariance with the rename proposed above) even if the variance increased. What you probably want is
```
if (Variance < MinVariance) {
  WithMinVariance = *ExpectedCounterValues;
  MinVariancec = Variance;
}
```


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:98
+
+  switch (ResultAggMode) {
+  case InstructionBenchmark::MinVariance: {
----------------
I'd still split this into two arguments:
One that decides what happens with the return values of runAndSample() with {Concatenate, MinVariance}.

And another one that decides what to do with the results of the previous step (Min, Max, Mean, return as is}.

With the current MinVariance filtering in all cases, we're changing the behavior for scalar counters, where we might drop some values (and we might drop some values also in the LBR case).


================
Comment at: llvm/tools/llvm-exegesis/lib/PerfHelper.cpp:155
 
-llvm::Expected<int64_t> Counter::readOrError() const {
+llvm::Expected<std::vector<int64_t>> Counter::readOrError() const {
   return llvm::make_error<llvm::StringError>("Not implemented",
----------------
This should also be llvm::SmallVector (please test with HAVE_LIBPFM undefined/false).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81050