[PATCH] D76921: [llvm-exegesis] 'Min' repetition mode

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

Thx for the patch !
I have a couple of suggestions but I'm onboard with the approach.

Please wait for comments from @courbet as well.



================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:108
+
+  for (const std::unique_ptr<const SnippetRepetitor> &Repetitor : Repetitors) {
+    // Assemble at least kMinInstructionsForSnippet instructions by repeating
----------------
The scope of the for loop is pretty big. I think it would be better to create a separate function:
```
for (const std::unique_ptr<const SnippetRepetitor> &Repetitor : Repetitors) {
  if (auto InstrBenchmarkOrErr = runOneBenchmark(InstrBenchmark)) {
    // On success, aggregate this run
    ...
  } else {
    // On error, extract the Error value and return it.
    return InstrBenchmarkOrErr.takeError();
  }
}
```

This way you also don't have to have the RAII cleaner.


================
Comment at: llvm/tools/llvm-exegesis/lib/SnippetRepetitor.cpp:113
     return std::make_unique<LoopSnippetRepetitor>(State);
+  default:
+    llvm_unreachable("Unknown RepetitionModeE enum");
----------------
I'd rather have an empty case for `InstructionBenchmark::AggregateMin` and not use `default`.
This will help if we ever add a new mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76921