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

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:60
+    std::vector<int64_t> CounterValues;
+    CounterValues.reserve(16); // Counter returns at most 16 entries.
     SmallVector<StringRef, 2> CounterNames;
----------------
Please get rif of the magic number. Why 16 ? what about `Counter->numValues()` ?


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:87
       }
-      CounterValue += Counter->read();
+      auto ValueOrError = Counter->readOrError();
+      if (ValueOrError) {
----------------
Please split this off to `void accumulateCounterValues(ValueOrError.get(), CounterValues)`


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:88
+      auto ValueOrError = Counter->readOrError();
+      if (ValueOrError) {
+        int i = 0;
----------------
[style]
```
if (!ValueOrError)
  return ValueOrError.takeError();
...
```


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:94
+          else
+            CounterValues[i] = CounterValues[i] + result;
+          ++i;
----------------
Why not `+=` ?


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:27
 
+static double squareStdev(const std::vector<int64_t> Values) {
+  if (Values.empty())
----------------
`ComputeVariance` ?


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:37
+  for (const auto &V : Values) {
+    Ret += V - Mean;
+  }
----------------
This is missing a square.


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:42
+
 Expected<std::vector<BenchmarkMeasure>> LatencyBenchmarkRunner::runMeasurements(
     const FunctionExecutor &Executor) const {
----------------
So computing the min and stddev across values in `runAndMeasureMulti()` requires them to be measuring the same thing. From the documentation it's not clear to me what the values represent. Are they always homogeneous ? Can you give an example of what they are in the LBR case.


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:48
+  std::vector<int64_t> WithMinStdev;
+  double Stdev = std::numeric_limits<int64_t>::max();
   const char *CounterName = State.getPfmCounters().CycleCounter;
----------------
why not `std::numeric_limits<double>` ?


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:56
+    // We'll keep the reading with lowest stdev (ie., most stable)
+    double CurStdev = 0;
+    if (WithMinStdev.empty() ||
----------------
Technically you're computing a variance.


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:57
+    double CurStdev = 0;
+    if (WithMinStdev.empty() ||
+        Stdev < (CurStdev = squareStdev(*ExpectedCounterValue))) {
----------------
Because of short-circuiting, if `WithMinStdev.empty()`, then `CurStdev` will not be evaluated, and `CurStdev` will be zero. Then `Stdev` will be set to `0`, preventing any further updates.


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:58
+    if (WithMinStdev.empty() ||
+        Stdev < (CurStdev = squareStdev(*ExpectedCounterValue))) {
+      WithMinStdev = *ExpectedCounterValue;
----------------
Was this supposed to be the other way around ? Here we are selecting the largest stddev.


================
Comment at: llvm/tools/llvm-exegesis/lib/PerfHelper.cpp:136
 
-  return Count;
+  return std::vector<int64_t>{Count};
 }
----------------
This returns a vector with `Count` elements set to zero. If this passes all tests then we are clearly missing tests :(



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81050