From a3400191137e7c991b89ebe72211790dae1648a1 Mon Sep 17 00:00:00 2001 From: Roman Lebedev Date: Sun, 18 Dec 2022 03:44:13 +0300 Subject: [PATCH] [llvm-exegesis] Unbreak `--benchmarks-file=` 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... --- .../llvm-exegesis/X86/latency/latency-LEA64_32r.s | 18 ++++++++++++++++++ llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp | 19 ------------------- llvm/tools/llvm-exegesis/lib/BenchmarkResult.h | 5 +++-- llvm/tools/llvm-exegesis/llvm-exegesis.cpp | 13 ++++++++++++- .../tools/llvm-exegesis/Mips/BenchmarkResultTest.cpp | 12 ++++++++++-- .../tools/llvm-exegesis/X86/BenchmarkResultTest.cpp | 11 ++++++++++- 6 files changed, 53 insertions(+), 25 deletions(-) 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 82c2c315..37daea5 100644 --- 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 --- 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 --- 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 --- 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 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> 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 --- 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 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 --- 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 Buffer = std::move(*MemoryBuffer::getFile(Filename)); -- 2.7.4