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

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:90
+
+class LbrFunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor {
+public:
----------------
oontvoo wrote:
> courbet wrote:
> > Apart from the PerfEvent, this does not really have anything to do with LBR.
> > On the one hand, this is not really a full-fledged `FunctionExecutor`, as it just throws away the `CounterName` argument and always measures the latency. This would be an argument for moving this to the X86 backend.
> > On the other hand, this has a lot of target-independent code, so it's a good thing that other targets can use it. The main issue, and the only thing that's really specific to LBR here is the perf event. Letting the target create the perf event as well as the counter does fix this.
> > 
> > So what we can do:
> >  - change `Target::createCounter` to take a counter name rather than a `PerfEvent`. The default implementation creates the PerfEvent, validates it and wraps it in a `Counter`. The X86 implementation tests if we're asking for the cycles counter in LBR mode, and creates the relevant `PerfEvent` and `LBRCounter`. It falls back to the default implementation else.
> >  - We stop ignoring the counter name argument in `LbrFunctionExecutorImpl::runAndMeasure`, and just propagate it to the ExegesisTarget.
> >  - We rename `LbrFunctionExecutorImpl` into `WorkerThreadFunctionExecutorImpl` (and `FunctionExecutorImpl` could be renamed to  `SameThreadFunctionExecutor`, and everything becomes target agnostic.
> Done - instead of moving the executor out, I've just let the Target decides whether to use WorkerThread executor or otherwise. 
That forces passing the `RunArg` to the target, which has no reason to know about it. 

What I'm suggesting is the following:
 - move `LbrSamplePeriod` to `X86/Target.cpp`. This whole `SamplePeriod` thing, and the existence of the option is really just a X86 detail, the target-independent code has no need to know about any of this.
 - Get rid of `SamplePeriod`, in the target-independant code. `RunArg` can go too.
 - createCounter does not need `Description`, or `SamplePeriod`. The X86 target handles it internally.


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/Target.cpp:612
+    // Can't use LBR without HAVE_LIBPFM
+    if (strcmp(Description, "LBR") == 0) {
+#ifdef HAVE_LIBPFM
----------------
This should really onely be:

```
Expected<std::unique_ptr<pfm::Counter>>
createCounter(const char *CounterName) {
  if (CounterName == kCyclesCounter && LbrSamplingPeriod > 0) {
    return std::make_unique<X86LbrCounter>(PerfEvent("LBR", LbrSamplingPeriod));
  }
  return ExegesisTarget::createCounter(CounterName);
}


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/Target.cpp:614
+#ifdef HAVE_LIBPFM
+      return std::make_unique<X86LbrCounter>(PerfEvent("LBR", SamplingPeriod));
+#else
----------------
This will fail to compile under windows because `X86LbrCounter` does not exist there.


================
Comment at: llvm/tools/llvm-exegesis/llvm-exegesis.cpp:348
 
+#if !defined(__x86_64__) && !defined(__x86__) && !defined(__i386__)
+  if (exegesis::BenchmarkMode == exegesis::InstructionBenchmark::Latency &&
----------------
This compile-time guarded runtime check can now go, because the x86 specific concepts are handled in the x86 target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77422