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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet 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:
> 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();


```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76921