[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/docs/CommandGuide/llvm-exegesis.rst:197
 
+ `latency` mode can be  make use of either RDTSC or LBR.
+ `latency[LBR]` is only available on X86 (at least `Skylake`).
----------------
`On X86, `


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:13
 #include <string>
+#include <thread>
 
----------------
you're no longer using `<thread>` and `<mutex>`.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:16
 #include "Assembler.h"
+#include "BenchmarkResult.h"
 #include "BenchmarkRunner.h"
----------------
is this needed ?


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:63
+
+      if (!CounterOrError) {
+        return CounterOrError.takeError();
----------------
[style] no braces for 1-line conditional bodies


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:66
+      }
+      auto *Counter = CounterOrError.get().get();
       Scratch->clear();
----------------
`s/auto/pfm::Counter/`


================
Comment at: llvm/tools/llvm-exegesis/lib/PerfHelper.h:45
   PerfEvent(PerfEvent &&other);
-  ~PerfEvent();
+  virtual ~PerfEvent();
 
----------------
why is the dtor virtual ? I don't see any virtual methods ?


================
Comment at: llvm/tools/llvm-exegesis/lib/Target.cpp:36
+  pfm::PerfEvent Event(CounterName);
+  if (!Event.valid()) {
+    return llvm::make_error<Failure>(
----------------
[style] no braces for 1-line conditional bodies


================
Comment at: llvm/tools/llvm-exegesis/lib/Target.h:69
 
+  virtual Expected<std::unique_ptr<pfm::Counter>>
+  createCounter(const char *CounterName, const LLVMState &State) const;
----------------
Can you submit this change independently and rebase ? Thanks !


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/Target.cpp:595
+                const LLVMState &State) const override {
+    if (CounterName == State.getPfmCounters().CycleCounter) {
+      // Can't use LBR without HAVE_LIBPFM, or __linux__ (for now)
----------------
`&& LbrSamplingPeriod > 0` ?


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:64
+
+// Parses the given data-buffer for stats and fill the CycleArray.
+// If data has been extracted successfully, also modifies the code to jump
----------------
`fills`


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:87
+      CycleArray->push_back(Entry.cycles);
+      if (i == Count - 1) {
+        // We've reached the last entry.
----------------
[style] no braces for 1-line conditional bodies


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:130
+                       PROT_READ | PROT_WRITE, MAP_SHARED, FileDescriptor, 0);
+  if (MMappedBuffer == MAP_FAILED) {
+    llvm::errs() << "Failed to mmap buffer.";
----------------
[style] no braces for 1-line conditional bodies


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:143
+  auto errorOrResult = readOrError();
+  if (errorOrResult) {
+    return errorOrResult.get();
----------------
[style] no braces for 1-line conditional bodies


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:183
+                                                 llvm::errc::io_error);
+    } else if (PollResult == 0) {
+      llvm::errs() << "LBR polling timed out without result, NumTimeouts ="
----------------
[style]

```
if (PollResult == -1)
  return llvm::make_error<llvm::StringError>("Cannot poll LBR perf event.", llvm::errc::io_error);
if (PollResult != 0) 
  continue;
...
```


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:184
+    } else if (PollResult == 0) {
+      llvm::errs() << "LBR polling timed out without result, NumTimeouts ="
+                   << NumTimeouts << ". ";
----------------
how often does that happen ? Do we want that much logging ?


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:205
+  // (Eg., check the cap bits another at the beginning of the benchmark)
+  if (!CheckLbrFormat(Page)) {
+    return llvm::make_error<llvm::StringError>("Unexpected LBR format",
----------------
[style] no braces for 1-line  conditional bodies


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:215
+  const size_t DataSize = DataHead - DataTail;
+  if (DataSize > kDataBufferSize) {
+    return llvm::make_error<llvm::StringError>(
----------------
ditto


================
Comment at: llvm/tools/llvm-exegesis/lib/X86/X86Counter.cpp:222
+
+  if (!error) {
+    // FIXME Rework the Counter interface to allow returning more than one reads
----------------
ditto


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

https://reviews.llvm.org/D77422