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

[Ondrej Sykora via Phabricator via llvm-commits]

ondrasej added a comment.

This looks good in general, but we should be careful about aggregating the values from the measurements (and aggregation when the counter returns multiple values). In particular for the LBR, we'd be losing interesting and potentially useful information by aggregating all the values into a single number.



================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.h:71
+
+    virtual Expected<std::vector<int64_t>>
+    runAndMeasureMulti(const char *Counters) const = 0;
----------------
This should be llvm::SmallVector<int64_t, N> with some small N (e.g. 4) to avoid memory allocations when there is just one return value. This is the case for virtually all counters except the LBR.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.h:72
+    virtual Expected<std::vector<int64_t>>
+    runAndMeasureMulti(const char *Counters) const = 0;
   };
----------------
Consider calling this runAndSample().


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:32
+  for (const auto &V : Values)
+    Sum += V;
+
----------------
You could do
const double Sum = std::accumulate(Values.begin(), Values.end(), 0.0);
instead.


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:68
+      MinValue = V;
+  }
+
----------------
I'd prefer to have more flexibility about the numbers that are returned and reported by the tool. The original code collapsed the measurements to a single value (the minimum). This is useful when looking for lower bounds/optimistic numbers, but other processing would also make sense:
- the mean of the measurements - this might give a better idea of the actual performance when running in the loop.
- the list of all measured values - so that the user can analyze the distribution of the measurements by themselves (and go beyond the mean/variance).

In particular for the LBR measurements: being able to see the raw timings of individual loop iterations is what makes that measurement method so appealing, and we'd lose all that information if we just took an aggregate value, be it a min or a mean. The min and mean from the LBR do have their value and I'd expect them to be more precise than the measurements over an unrolled loop, so ideally we'd have a way to see both.

I think a good solution would be to add an argument to this method that determines how the values are aggregated over the measurements:
- min,
- mean,
- min variance,
- keep all.

 and over the contents of the returned vector:
- min,
- mean,
- keep all.

This will mean more changes up the call stack, but it would give us a lot more flexibility in using the tool. What do you think?


================
Comment at: llvm/tools/llvm-exegesis/lib/PerfHelper.cpp:123
   if (ValueOrError)
-    return ValueOrError.get();
+    return ValueOrError.get()[0];
 
----------------
You might want to check that there is at least one element.


================
Comment at: llvm/tools/llvm-exegesis/lib/PerfHelper.cpp:129
 
-llvm::Expected<int64_t> Counter::readOrError() const {
+llvm::Expected<std::vector<int64_t>> Counter::readOrError() const {
   int64_t Count = 0;
----------------
This should be llvm::SmallVector<int64_t, some small N> - most counters return just a single value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81050