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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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
----------------
gchatelet wrote:
> 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.
Hmm. I agree that the loop is getting big, but are you very sure about the fix?

Like i said in the patch's description, i tried a variation of that approach
initially, and i'd say the result is much uglier. Instead of having one place
that fills a single `InstrBenchmark` and just ensuring it doesn't completely
override previous results, we will now need to keep two places in sync.

Please do note that we have two types of errors here,
a fatal one that we signal via `Error`, and a measurement-time error,
that we squash and stash into `InstrBenchmark.Error`. The latter
must not be reported as a fatal error, but it must interrupt aggregation.
So we won't avoid the RAII, just change it's form.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76921