[llvm-exegesis] Unbreak `--benchmarks-file=<f>`

I'm absolutely flabbergasted by this.
I was absolutely sure this worked.
But apparently not.

When outputting to the file, we'd write a single `InstructionBenchmark`
to it, and then close the file. And for the next `InstructionBenchmark`,
we'd reopen the file, truncating it in process,
and thus the first `InstructionBenchmark` would be gone...

diff --git a/llvm/test/tools/llvm-exegesis/X86/latency/latency-LEA64_32r.s b/llvm/test/tools/llvm-exegesis/X86/latency/latency-LEA64_32r.s
index 82c2c31..37daea5 100644 (file)
--- a/llvm/test/tools/llvm-exegesis/X86/latency/latency-LEA64_32r.s
+++ b/llvm/test/tools/llvm-exegesis/X86/latency/latency-LEA64_32r.s
@@ -1,5 +1,19 @@
 # RUN: llvm-exegesis -mtriple=x86_64-unknown-unknown -mcpu=x86-64 -mode=latency --skip-measurements -opcode-name=LEA64_32r -repetition-mode=duplicate -max-configs-per-opcode=2 | FileCheck %s
+# RUN: llvm-exegesis -mtriple=x86_64-unknown-unknown -mcpu=x86-64 -mode=latency --skip-measurements -opcode-name=LEA64_32r -repetition-mode=duplicate -max-configs-per-opcode=2 | FileCheck --check-prefix CHECK-COUNTS %s
+
 # RUN: llvm-exegesis -mtriple=x86_64-unknown-unknown -mcpu=x86-64 -mode=latency --skip-measurements -opcode-name=LEA64_32r -repetition-mode=loop -max-configs-per-opcode=2 | FileCheck %s
+# RUN: llvm-exegesis -mtriple=x86_64-unknown-unknown -mcpu=x86-64 -mode=latency --skip-measurements -opcode-name=LEA64_32r -repetition-mode=loop -max-configs-per-opcode=2 | FileCheck --check-prefix CHECK-COUNTS %s
+
+## Intentionally run llvm-exegesis twice per output!
+# RUN: llvm-exegesis -mtriple=x86_64-unknown-unknown -mcpu=x86-64 -mode=latency --skip-measurements -opcode-name=LEA64_32r -repetition-mode=duplicate -max-configs-per-opcode=2 --benchmarks-file=%t.duplicate.yaml
+# RUN: llvm-exegesis -mtriple=x86_64-unknown-unknown -mcpu=x86-64 -mode=latency --skip-measurements -opcode-name=LEA64_32r -repetition-mode=duplicate -max-configs-per-opcode=2 --benchmarks-file=%t.duplicate.yaml
+# RUN: FileCheck --input-file %t.duplicate.yaml %s
+# RUN: FileCheck --input-file %t.duplicate.yaml --check-prefix CHECK-COUNTS %s
+# RUN: llvm-exegesis -mtriple=x86_64-unknown-unknown -mcpu=x86-64 -mode=latency --skip-measurements -opcode-name=LEA64_32r -repetition-mode=loop -max-configs-per-opcode=2 --benchmarks-file=%t.loop.yaml
+# RUN: llvm-exegesis -mtriple=x86_64-unknown-unknown -mcpu=x86-64 -mode=latency --skip-measurements -opcode-name=LEA64_32r -repetition-mode=loop -max-configs-per-opcode=2 --benchmarks-file=%t.loop.yaml
+# RUN: FileCheck --input-file %t.loop.yaml %s
+# RUN: FileCheck --input-file %t.loop.yaml --check-prefix CHECK-COUNTS %s
+
 
 CHECK:      ---
 CHECK-NEXT: mode: latency
@@ -14,3 +28,7 @@ CHECK-NEXT: key:
 CHECK-NEXT:   instructions:
 CHECK-NEXT:     LEA64_32r
 CHECK-NEXT: config: '42(%[[REG2:[A-Z0-9]+]], %[[REG2]], 1)'
+
+## Check that we empty our output file once per llvm-exegesis run.
+## But not once per benchmark.
+CHECK-COUNTS-COUNT-2: mode: latency

diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp b/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
index 593b41e..0478967 100644 (file)
--- a/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
+++ b/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
@@ -394,25 +394,6 @@ Error InstructionBenchmark::readYamlFrom(const LLVMState &State,
   return Error::success();
 }
 
-Error InstructionBenchmark::writeYaml(const LLVMState &State,
-                                      const StringRef Filename) {
-  if (Filename == "-") {
-    if (auto Err = writeYamlTo(State, outs()))
-      return Err;
-  } else {
-    int ResultFD = 0;
-    if (auto E = errorCodeToError(openFileForWrite(Filename, ResultFD,
-                                                   sys::fs::CD_CreateAlways,
-                                                   sys::fs::OF_TextWithCRLF))) {
-      return E;
-    }
-    raw_fd_ostream Ostr(ResultFD, true /*shouldClose*/);
-    if (auto Err = writeYamlTo(State, Ostr))
-      return Err;
-  }
-  return Error::success();
-}
-
 void PerInstructionStats::push(const BenchmarkMeasure &BM) {
   if (Key.empty())
     Key = BM.Key;

diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkResult.h b/llvm/tools/llvm-exegesis/lib/BenchmarkResult.h
index ce61b87..679946d 100644 (file)
--- a/llvm/tools/llvm-exegesis/lib/BenchmarkResult.h
+++ b/llvm/tools/llvm-exegesis/lib/BenchmarkResult.h
@@ -100,9 +100,10 @@ struct InstructionBenchmark {
   class Error readYamlFrom(const LLVMState &State, StringRef InputContent);
 
   // Write functions, non-const because of YAML traits.
+  // NOTE: we intentionally do *NOT* have a variant of this function taking
+  //       filename, because it's behaviour is bugprone with regards to
+  //       accidentally using it more than once and overriding previous YAML.
   class Error writeYamlTo(const LLVMState &State, raw_ostream &S);
-
-  class Error writeYaml(const LLVMState &State, const StringRef Filename);
 };
 
 bool operator==(const BenchmarkMeasure &A, const BenchmarkMeasure &B);

diff --git a/llvm/tools/llvm-exegesis/llvm-exegesis.cpp b/llvm/tools/llvm-exegesis/llvm-exegesis.cpp
index 7f9cd39..e74fb15 100644 (file)
--- a/llvm/tools/llvm-exegesis/llvm-exegesis.cpp
+++ b/llvm/tools/llvm-exegesis/llvm-exegesis.cpp
@@ -401,6 +401,17 @@ void benchmarkMain() {
   if (BenchmarkFile.empty())
     BenchmarkFile = "-";
 
+  std::optional<raw_fd_ostream> FileOstr;
+  if (BenchmarkFile != "-") {
+    int ResultFD = 0;
+    // Create output file or open existing file and truncate it, once.
+    ExitOnErr(errorCodeToError(openFileForWrite(BenchmarkFile, ResultFD,
+                                                sys::fs::CD_CreateAlways,
+                                                sys::fs::OF_TextWithCRLF)));
+    FileOstr.emplace(ResultFD, true /*shouldClose*/);
+  }
+  raw_ostream &Ostr = FileOstr ? *FileOstr : outs();
+
   std::optional<ProgressMeter<>> Meter;
   if (BenchmarkMeasurementsPrintProgress)
     Meter.emplace(Configurations.size());
@@ -440,7 +451,7 @@ void benchmarkMain() {
       }
     }
 
-    ExitOnFileError(BenchmarkFile, Result.writeYaml(State, BenchmarkFile));
+    ExitOnFileError(BenchmarkFile, Result.writeYamlTo(State, Ostr));
   }
   exegesis::pfm::pfmTerminate();
 }

diff --git a/llvm/unittests/tools/llvm-exegesis/Mips/BenchmarkResultTest.cpp b/llvm/unittests/tools/llvm-exegesis/Mips/BenchmarkResultTest.cpp
index 3b7d248..429e658 100644 (file)
--- a/llvm/unittests/tools/llvm-exegesis/Mips/BenchmarkResultTest.cpp
+++ b/llvm/unittests/tools/llvm-exegesis/Mips/BenchmarkResultTest.cpp
@@ -78,8 +78,16 @@ TEST_F(MipsBenchmarkResultTest, WriteToAndReadFromDisk) {
   SmallString<64> Filename(TestDirectory.path());
   sys::path::append(Filename, "data.yaml");
   errs() << Filename << "-------\n";
-  ExitOnErr(ToDisk.writeYaml(State, Filename));
-
+  {
+    int ResultFD = 0;
+    // Create output file or open existing file and truncate it, once.
+    ExitOnErr(errorCodeToError(openFileForWrite(Filename, ResultFD,
+                                                sys::fs::CD_CreateAlways,
+                                                sys::fs::OF_TextWithCRLF)));
+    raw_fd_ostream FileOstr(ResultFD, true /*shouldClose*/);
+
+    ExitOnErr(ToDisk.writeYamlTo(State, FileOstr));
+  }
   const std::unique_ptr<MemoryBuffer> Buffer =
       std::move(*MemoryBuffer::getFile(Filename));
 

diff --git a/llvm/unittests/tools/llvm-exegesis/X86/BenchmarkResultTest.cpp b/llvm/unittests/tools/llvm-exegesis/X86/BenchmarkResultTest.cpp
index ba4efab..0d47a76 100644 (file)
--- a/llvm/unittests/tools/llvm-exegesis/X86/BenchmarkResultTest.cpp
+++ b/llvm/unittests/tools/llvm-exegesis/X86/BenchmarkResultTest.cpp
@@ -89,7 +89,16 @@ TEST(BenchmarkResultTest, WriteToAndReadFromDisk) {
   ASSERT_FALSE(EC);
   sys::path::append(Filename, "data.yaml");
   errs() << Filename << "-------\n";
-  ExitOnErr(ToDisk.writeYaml(State, Filename));
+  {
+    int ResultFD = 0;
+    // Create output file or open existing file and truncate it, once.
+    ExitOnErr(errorCodeToError(openFileForWrite(Filename, ResultFD,
+                                                sys::fs::CD_CreateAlways,
+                                                sys::fs::OF_TextWithCRLF)));
+    raw_fd_ostream FileOstr(ResultFD, true /*shouldClose*/);
+
+    ExitOnErr(ToDisk.writeYamlTo(State, FileOstr));
+  }
 
   const std::unique_ptr<MemoryBuffer> Buffer =
       std::move(*MemoryBuffer::getFile(Filename));