[PATCH] D53371: [llvm-exegesis] Allow measuring several instructions in a single run.

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: tools/llvm-exegesis/lib/BenchmarkRunner.cpp:58
+private:
+  llvm::Expected<double> runAndMeasure(const char *Counters) const override {
+    using llvm::CrashRecoveryContext;
----------------
Return an `int64_t` here.


================
Comment at: tools/llvm-exegesis/lib/Latency.cpp:91
   constexpr const int NumMeasurements = 30;
   int64_t MinLatency = std::numeric_limits<int64_t>::max();
   const char *CounterName = getCounterName();
----------------
Can you please rename `MinLatency` to `MinValue`?


================
Comment at: tools/llvm-exegesis/lib/Latency.cpp:96
   for (size_t I = 0; I < NumMeasurements; ++I) {
-    pfm::Counter Counter(CyclesPerfEvent);
-    Scratch.clear();
-    Counter.start();
-    Function(Scratch.ptr());
-    Counter.stop();
-    const int64_t Value = Counter.read();
-    if (Value < MinLatency)
-      MinLatency = Value;
+    auto CounterValue = Executor.runAndMeasure(CounterName);
+    if (!CounterValue) {
----------------
`ExpectedCounterValue`? so it's easier to understand the next lines


================
Comment at: tools/llvm-exegesis/lib/Latency.cpp:97
+    auto CounterValue = Executor.runAndMeasure(CounterName);
+    if (!CounterValue) {
+      return CounterValue.takeError();
----------------
remove the braces


================
Comment at: tools/llvm-exegesis/lib/Latency.cpp:101
+    if (*CounterValue < MinLatency)
+      MinLatency = static_cast<int64_t>(*CounterValue);
   }
----------------
Now you don't need to cast anymore.


================
Comment at: tools/llvm-exegesis/lib/Uops.cpp:236
+    auto CounterValue = Executor.runAndMeasure(Counters);
+    if (!CounterValue) {
+      return CounterValue.takeError();
----------------
remove the braces


================
Comment at: tools/llvm-exegesis/lib/Uops.cpp:245
+    auto CounterValue = Executor.runAndMeasure(UopsCounter);
+    if (!CounterValue) {
+      return CounterValue.takeError();
----------------
ditto


================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:121
+    std::vector<unsigned> Result(MCInstrInfo.getNumOpcodes());
+    std::iota(Result.begin(), Result.end(), 1);
+    return Result;
----------------
Let's go with a for loop so it's easier to reason about the last element.


================
Comment at: tools/llvm-exegesis/llvm-exegesis.cpp:125
   // Resolve opcode name -> opcode.
-  for (unsigned I = 0, E = MCInstrInfo.getNumOpcodes(); I < E; ++I)
-    if (MCInstrInfo.getName(I) == OpcodeName)
-      return I;
-  llvm::report_fatal_error(llvm::Twine("unknown opcode ").concat(OpcodeName));
+  const auto ResolveName = [&MCInstrInfo](llvm::StringRef OpcodeName) {
+    for (unsigned I = 1, E = MCInstrInfo.getNumOpcodes(); I < E; ++I)
----------------
can you specify the lambda return type so it's explicit.


Repository:
  rL LLVM

https://reviews.llvm.org/D53371