[llvm-exegesis]Allow clients to do their own snippet running error ha…

[legrosbuffle]

…ndling.

Returns an error and a benchmark rather than an error or a benchmark. This allows users to have custom error handling while still being able to inspect the benchmark.

Apart from this small API change, this is an NFC.

This is an alternative to #74211.

[llvmbot]

@llvm/pr-subscribers-tools-llvm-exegesis

Author: Clement Courbet (legrosbuffle)

Changes

…ndling.

Returns an error and a benchmark rather than an error or a benchmark. This allows users hanve custom error handling while still being able to inspect the benchmark.

Apart from this small API change, this is an NFC.

This is an alternative to #74211.

---

Full diff: https://github.com/llvm/llvm-project/pull/74711.diff

3 Files Affected:

• (modified) llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp (+6-10)
• (modified) llvm/tools/llvm-exegesis/lib/BenchmarkRunner.h (+1-1)
• (modified) llvm/tools/llvm-exegesis/llvm-exegesis.cpp (+12-2)

diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
index 8e242cdd1d905..6c34446e8d663 100644
--- a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
+++ b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
@@ -572,7 +572,7 @@ BenchmarkRunner::createFunctionExecutor(
   llvm_unreachable("ExecutionMode is outside expected range");
 }
 
-Expected<Benchmark> BenchmarkRunner::runConfiguration(
+std::pair<Error, Benchmark> BenchmarkRunner::runConfiguration(
     RunnableConfiguration &&RC,
     const std::optional<StringRef> &DumpFile) const {
   Benchmark &InstrBenchmark = RC.InstrBenchmark;
@@ -583,8 +583,7 @@ Expected<Benchmark> BenchmarkRunner::runConfiguration(
     auto ObjectFilePath =
         writeObjectFile(ObjectFile.getBinary()->getData(), *DumpFile);
     if (Error E = ObjectFilePath.takeError()) {
-      InstrBenchmark.Error = toString(std::move(E));
-      return std::move(InstrBenchmark);
+      return {std::move(E), std::move(InstrBenchmark)};
     }
     outs() << "Check generated assembly with: /usr/bin/objdump -d "
            << *ObjectFilePath << "\n";
@@ -592,20 +591,17 @@ Expected<Benchmark> BenchmarkRunner::runConfiguration(
 
   if (BenchmarkPhaseSelector < BenchmarkPhaseSelectorE::Measure) {
     InstrBenchmark.Error = "actual measurements skipped.";
-    return std::move(InstrBenchmark);
+    return {Error::success(), std::move(InstrBenchmark)};
   }
 
   Expected<std::unique_ptr<BenchmarkRunner::FunctionExecutor>> Executor =
       createFunctionExecutor(std::move(ObjectFile), RC.InstrBenchmark.Key);
   if (!Executor)
-    return Executor.takeError();
+    return {Executor.takeError(), std::move(InstrBenchmark)};
   auto NewMeasurements = runMeasurements(**Executor);
 
   if (Error E = NewMeasurements.takeError()) {
-    if (!E.isA<SnippetCrash>())
-      return std::move(E);
-    InstrBenchmark.Error = toString(std::move(E));
-    return std::move(InstrBenchmark);
+    return {std::move(E), std::move(InstrBenchmark)};
   }
   assert(InstrBenchmark.NumRepetitions > 0 && "invalid NumRepetitions");
   for (BenchmarkMeasure &BM : *NewMeasurements) {
@@ -618,7 +614,7 @@ Expected<Benchmark> BenchmarkRunner::runConfiguration(
   }
   InstrBenchmark.Measurements = std::move(*NewMeasurements);
 
-  return std::move(InstrBenchmark);
+  return {Error::success(), std::move(InstrBenchmark)};
 }
 
 Expected<std::string>
diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.h b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.h
index 24f2086289408..2c48d07e37ca9 100644
--- a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.h
+++ b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.h
@@ -65,7 +65,7 @@ class BenchmarkRunner {
                            unsigned NumRepetitions, unsigned LoopUnrollFactor,
                            const SnippetRepetitor &Repetitor) const;
 
-  Expected<Benchmark>
+  std::pair<Error, Benchmark>
   runConfiguration(RunnableConfiguration &&RC,
                    const std::optional<StringRef> &DumpFile) const;
 
diff --git a/llvm/tools/llvm-exegesis/llvm-exegesis.cpp b/llvm/tools/llvm-exegesis/llvm-exegesis.cpp
index 4e466303c9dbf..148891a18246f 100644
--- a/llvm/tools/llvm-exegesis/llvm-exegesis.cpp
+++ b/llvm/tools/llvm-exegesis/llvm-exegesis.cpp
@@ -410,8 +410,18 @@ static void runBenchmarkConfigurations(
       std::optional<StringRef> DumpFile;
       if (DumpObjectToDisk.getNumOccurrences())
         DumpFile = DumpObjectToDisk;
-      AllResults.emplace_back(
-          ExitOnErr(Runner.runConfiguration(std::move(RC), DumpFile)));
+      auto [Err, InstrBenchmark] =
+          Runner.runConfiguration(std::move(RC), DumpFile);
+      if (Err) {
+        // Errors from executing the snippets are fine.
+        // All other errors are a framework issue and should fail.
+        if (!Err.isA<SnippetCrash>()) {
+          llvm::errs() << "llvm-exegesis error: " << toString(std::move(Err));
+          exit(1);
+        }
+        InstrBenchmark.Error = toString(std::move(Err));
+      }
+      AllResults.push_back(std::move(InstrBenchmark));
     }
     Benchmark &Result = AllResults.front();
 

[boomanaiden154]

LGTM. Thanks for putting this together! This is definitely significantly better than the hacky approach I proposed.