[PATCH] D77422: [llvm-exegesis] Add benchmark mode that uses LBR for more precise measurements.

[Clement Courbet via Phabricator via llvm-commits]

courbet added a comment.

> This isn't a new mode, it's still for measuring latency.

This is a good point. In the end what matters is that we're measuring the same thing, and e.g. analysis should not care how we're measuring  this value.



================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:106
+    pfm::PerfEvent PerfEvent("LBR", LbrSamplePeriod);
+    pfm::Counter Counter = State.getExegesisTarget().createCounter(PerfEvent);
+    {
----------------
Does'n this slice the `Counter` object ?


================
Comment at: llvm/tools/llvm-exegesis/lib/PerfHelper.cpp:91
+void PerfEvent::initPerfEventForLbr() {
+  Attr = new perf_event_attr();
+  *Attr = {0};
----------------
Should this not be guarded  by `HAVE_LIBPFM` ? Can you please try to compile & run without libpfm support to validate ?


================
Comment at: llvm/tools/llvm-exegesis/lib/Target.cpp:109
+std::unique_ptr<BenchmarkRunner>
+ExegesisTarget::createLbrLatencyBenchmarkRunner(const LLVMState &State) const {
+#if defined(__x86_64__) || defined(__x86__) || defined(__i386__)
----------------
This shold move to the X86 exegesis target. The function is virtual, so the default implementation can return false.

And with Roman's suggestion, you don't need it at all :)


================
Comment at: llvm/tools/llvm-exegesis/lib/Target.h:182
       const LLVMState &State) const;
+  std::unique_ptr<BenchmarkRunner> virtual createLbrLatencyBenchmarkRunner(
+      const LLVMState &State) const;
----------------
With Roman's suggestion, you can just remove this and use createLatencyBenchmarkRunner for this.


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/Target.cpp:578
 namespace {
+#ifdef HAVE_LIBPFM
+class X86LbrCounter : public pfm::Counter {
----------------
Please move this to a separate file within X86 the target.


================
Comment at: llvm/tools/llvm-exegesis/llvm-exegesis.cpp:88
 
+static cl::opt<unsigned> LbrSamplePeriod(
+    "lbr-sample-period",
----------------
Let's move this to the X86 target, and rename it to `x86-lbr-sample-period`, with a default of 0. If non-zero, we're going to use LBR. Else, we'll use the non-LBR runner.


================
Comment at: llvm/tools/llvm-exegesis/llvm-exegesis.cpp:91
+    cl::desc("The sample period (nbranches/sample), used for LBR sampling"),
+    cl::cat(BenchmarkOptions), cl::init(521)); // 521 chosen based on GWP
+
----------------
Please remove references to GWP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77422