[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:
> lebedev.ri wrote:
> > 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.
> Acknowledge.
> I'm not a huge fan of the RAII cleaner which spans the whole function and needs comments to be understood.
> Maybe a custom struct would convey more semantics, it's not a lot more code.
> ```
> struct ClearBenchmarkOnReturn {
>   ClearBenchmarkOnReturn(InstructionBenchmark* IB) : IB(IB)
>   ~ClearBenchmarkOnReturn() { if(Clear) IB->Measurements.clear(); }
> 
>   void disarm() { Clear = false; }
> private:
>   InstructionBenchmark* const IB;
>   bool Clear = true;
> };
> ----------------------
> 
> ClearBenchmarkOnReturn CBOR(&InstrBenchmark);
> 
> ...
> 
> CBOR.disarm();
> 
> 
> ```
Ok, sure, that i can do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76921