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

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo marked an inline comment as done.
oontvoo added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/LatencyBenchmarkRunner.cpp:68
+      MinValue = V;
+  }
+
----------------
ondrasej wrote:
> 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?
Yes, that's a good idea. I've thought about this a bit more and realised even keeping min-variance (of each read) could still be completely wrong.

Imagine your benchmarked code has a number of distinct branches; then there is no reason to expect the cycles from these branches to correlate. The min-variance approach is only meaningful if it's the same code-path. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81050