[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
----------------
lebedev.ri wrote:
> 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.
> 
> override previous results, we will now need to keep two places in sync.

Err, to be more specific:
We will then need to just rename the existing `BenchmarkRunner::runConfiguration()`
as that `runOneBenchmark()`, and new `BenchmarkRunner::runConfiguration()`
would need to know how to aggregate every field of fresh `InstructionBenchmark`
returned from `BenchmarkRunner::runOneBenchmark()` into the 'aggregate' report.
This seems more complex than teaching `BenchmarkRunner::runConfiguration()`
itself to do just that within the per-Repetitor loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76921