[llvm] a340019 - [llvm-exegesis] Unbreak `--benchmarks-file=<f>`
Roman Lebedev via llvm-commits
llvm-commits at lists.llvm.org
Sat Dec 17 16:50:35 PST 2022
Author: Roman Lebedev
Date: 2022-12-18T03:50:10+03:00
New Revision: a3400191137e7c991b89ebe72211790dae1648a1
URL: https://github.com/llvm/llvm-project/commit/a3400191137e7c991b89ebe72211790dae1648a1
DIFF: https://github.com/llvm/llvm-project/commit/a3400191137e7c991b89ebe72211790dae1648a1.diff
LOG: [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...
Added:
Modified:
llvm/test/tools/llvm-exegesis/X86/latency/latency-LEA64_32r.s
llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
llvm/tools/llvm-exegesis/lib/BenchmarkResult.h
llvm/tools/llvm-exegesis/llvm-exegesis.cpp
llvm/unittests/tools/llvm-exegesis/Mips/BenchmarkResultTest.cpp
llvm/unittests/tools/llvm-exegesis/X86/BenchmarkResultTest.cpp
Removed:
################################################################################
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 82c2c31585a4..37daea530ab0 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 593b41eb70df..0478967ace64 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 ce61b870056e..679946d723e0 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 7f9cd3991cf5..e74fb155d944 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<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 3b7d248e5b40..429e6589eb5d 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<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 ba4efabcab18..0d47a76b5480 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<MemoryBuffer> Buffer =
std::move(*MemoryBuffer::getFile(Filename));
More information about the llvm-commits
mailing list