Revert "[Exegesis] Add the ability to dry-run the measurement phase (… #122371
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lvm#121991)" This reverts commit f8f8598.
|
@llvm/pr-subscribers-tools-llvm-exegesis @llvm/pr-subscribers-llvm-binary-utilities Author: Min-Yih Hsu (mshockwave) Changes…#121991)" This reverts commit f8f8598. This breaks ARMv7 and s390x buildbot with the following message: Which I'm a little confused because I'm sure it's coming from Full diff: https://github.com/llvm/llvm-project/pull/122371.diff 6 Files Affected:
diff --git a/llvm/docs/CommandGuide/llvm-exegesis.rst b/llvm/docs/CommandGuide/llvm-exegesis.rst
index d357c2ceea4189..8266d891a5e6b1 100644
--- a/llvm/docs/CommandGuide/llvm-exegesis.rst
+++ b/llvm/docs/CommandGuide/llvm-exegesis.rst
@@ -301,7 +301,6 @@ OPTIONS
* ``prepare-and-assemble-snippet``: Same as ``prepare-snippet``, but also dumps an excerpt of the sequence (hex encoded).
* ``assemble-measured-code``: Same as ``prepare-and-assemble-snippet``. but also creates the full sequence that can be dumped to a file using ``--dump-object-to-disk``.
* ``measure``: Same as ``assemble-measured-code``, but also runs the measurement.
- * ``dry-run-measurement``: Same as measure, but does not actually execute the snippet.
.. option:: --x86-lbr-sample-period=<nBranches/sample>
diff --git a/llvm/test/tools/llvm-exegesis/dry-run-measurement.test b/llvm/test/tools/llvm-exegesis/dry-run-measurement.test
deleted file mode 100644
index e4449d7df3d826..00000000000000
--- a/llvm/test/tools/llvm-exegesis/dry-run-measurement.test
+++ /dev/null
@@ -1,11 +0,0 @@
-# RUN: llvm-exegesis --mtriple=riscv64 --mcpu=sifive-p470 --mode=latency --opcode-name=ADD --use-dummy-perf-counters --benchmark-phase=dry-run-measurement | FileCheck %s
-# REQUIRES: riscv-registered-target
-
-# This test makes sure that llvm-exegesis doesn't execute "cross-compiled" snippets in the presence of
-# --dry-run-measurement. RISC-V was chosen simply because most of the time we run tests on X86 machines.
-
-# Should not contain misleading results.
-# CHECK: measurements: []
-
-# Should not contain error messages like "snippet crashed while running: Segmentation fault".
-# CHECK: error: ''
diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkResult.h b/llvm/tools/llvm-exegesis/lib/BenchmarkResult.h
index 5480d856168784..3c09a8380146e5 100644
--- a/llvm/tools/llvm-exegesis/lib/BenchmarkResult.h
+++ b/llvm/tools/llvm-exegesis/lib/BenchmarkResult.h
@@ -38,7 +38,6 @@ enum class BenchmarkPhaseSelectorE {
PrepareAndAssembleSnippet,
AssembleMeasuredCode,
Measure,
- DryRunMeasure,
};
enum class BenchmarkFilter { All, RegOnly, WithMem };
diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
index cc46f7feb6cf7f..a7771b99e97b1a 100644
--- a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
+++ b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
@@ -99,7 +99,7 @@ class InProcessFunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor {
static Expected<std::unique_ptr<InProcessFunctionExecutorImpl>>
create(const LLVMState &State, object::OwningBinary<object::ObjectFile> Obj,
BenchmarkRunner::ScratchSpace *Scratch,
- std::optional<int> BenchmarkProcessCPU, bool DryRun) {
+ std::optional<int> BenchmarkProcessCPU) {
Expected<ExecutableFunction> EF =
ExecutableFunction::create(State.createTargetMachine(), std::move(Obj));
@@ -107,17 +107,14 @@ class InProcessFunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor {
return EF.takeError();
return std::unique_ptr<InProcessFunctionExecutorImpl>(
- new InProcessFunctionExecutorImpl(State, std::move(*EF), Scratch,
- DryRun));
+ new InProcessFunctionExecutorImpl(State, std::move(*EF), Scratch));
}
private:
InProcessFunctionExecutorImpl(const LLVMState &State,
ExecutableFunction Function,
- BenchmarkRunner::ScratchSpace *Scratch,
- bool DryRun)
- : State(State), Function(std::move(Function)), Scratch(Scratch),
- DryRun(DryRun) {}
+ BenchmarkRunner::ScratchSpace *Scratch)
+ : State(State), Function(std::move(Function)), Scratch(Scratch) {}
static void accumulateCounterValues(const SmallVector<int64_t, 4> &NewValues,
SmallVector<int64_t, 4> *Result) {
@@ -146,14 +143,9 @@ class InProcessFunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor {
CrashRecoveryContext CRC;
CrashRecoveryContext::Enable();
const bool Crashed = !CRC.RunSafely([this, Counter, ScratchPtr]() {
- if (DryRun) {
- Counter->start();
- Counter->stop();
- } else {
- Counter->start();
- this->Function(ScratchPtr);
- Counter->stop();
- }
+ Counter->start();
+ this->Function(ScratchPtr);
+ Counter->stop();
});
CrashRecoveryContext::Disable();
PS.reset();
@@ -185,7 +177,6 @@ class InProcessFunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor {
const LLVMState &State;
const ExecutableFunction Function;
BenchmarkRunner::ScratchSpace *const Scratch;
- bool DryRun = false;
};
#ifdef __linux__
@@ -673,9 +664,6 @@ Expected<std::unique_ptr<BenchmarkRunner::FunctionExecutor>>
BenchmarkRunner::createFunctionExecutor(
object::OwningBinary<object::ObjectFile> ObjectFile,
const BenchmarkKey &Key, std::optional<int> BenchmarkProcessCPU) const {
- bool DryRun =
- BenchmarkPhaseSelector == BenchmarkPhaseSelectorE::DryRunMeasure;
-
switch (ExecutionMode) {
case ExecutionModeE::InProcess: {
if (BenchmarkProcessCPU.has_value())
@@ -683,8 +671,7 @@ BenchmarkRunner::createFunctionExecutor(
"support benchmark core pinning.");
auto InProcessExecutorOrErr = InProcessFunctionExecutorImpl::create(
- State, std::move(ObjectFile), Scratch.get(), BenchmarkProcessCPU,
- DryRun);
+ State, std::move(ObjectFile), Scratch.get(), BenchmarkProcessCPU);
if (!InProcessExecutorOrErr)
return InProcessExecutorOrErr.takeError();
@@ -692,10 +679,6 @@ BenchmarkRunner::createFunctionExecutor(
}
case ExecutionModeE::SubProcess: {
#ifdef __linux__
- if (DryRun)
- return make_error<Failure>("The subprocess execution mode cannot "
- "dry-run measurement at this moment.");
-
auto SubProcessExecutorOrErr = SubProcessFunctionExecutorImpl::create(
State, std::move(ObjectFile), Key, BenchmarkProcessCPU);
if (!SubProcessExecutorOrErr)
diff --git a/llvm/tools/llvm-exegesis/lib/Target.cpp b/llvm/tools/llvm-exegesis/lib/Target.cpp
index e2251ff978888b..29e58692f0e92b 100644
--- a/llvm/tools/llvm-exegesis/lib/Target.cpp
+++ b/llvm/tools/llvm-exegesis/lib/Target.cpp
@@ -98,7 +98,7 @@ ExegesisTarget::createBenchmarkRunner(
return nullptr;
case Benchmark::Latency:
case Benchmark::InverseThroughput:
- if (BenchmarkPhaseSelector >= BenchmarkPhaseSelectorE::Measure &&
+ if (BenchmarkPhaseSelector == BenchmarkPhaseSelectorE::Measure &&
!PfmCounters.CycleCounter) {
const char *ModeName = Mode == Benchmark::Latency
? "latency"
@@ -116,7 +116,7 @@ ExegesisTarget::createBenchmarkRunner(
State, Mode, BenchmarkPhaseSelector, ResultAggMode, ExecutionMode,
ValidationCounters, BenchmarkRepeatCount);
case Benchmark::Uops:
- if (BenchmarkPhaseSelector >= BenchmarkPhaseSelectorE::Measure &&
+ if (BenchmarkPhaseSelector == BenchmarkPhaseSelectorE::Measure &&
!PfmCounters.UopsCounter && !PfmCounters.IssueCounters)
return make_error<Failure>(
"can't run 'uops' mode, sched model does not define uops or issue "
diff --git a/llvm/tools/llvm-exegesis/llvm-exegesis.cpp b/llvm/tools/llvm-exegesis/llvm-exegesis.cpp
index 07bd44ee64f1f2..fa37e05956be8c 100644
--- a/llvm/tools/llvm-exegesis/llvm-exegesis.cpp
+++ b/llvm/tools/llvm-exegesis/llvm-exegesis.cpp
@@ -132,10 +132,7 @@ static cl::opt<BenchmarkPhaseSelectorE> BenchmarkPhaseSelector(
clEnumValN(
BenchmarkPhaseSelectorE::Measure, "measure",
"Same as prepare-measured-code, but also runs the measurement "
- "(default)"),
- clEnumValN(
- BenchmarkPhaseSelectorE::DryRunMeasure, "dry-run-measurement",
- "Same as measure, but does not actually execute the snippet")),
+ "(default)")),
cl::init(BenchmarkPhaseSelectorE::Measure));
static cl::opt<bool>
@@ -479,7 +476,7 @@ static void runBenchmarkConfigurations(
}
void benchmarkMain() {
- if (BenchmarkPhaseSelector >= BenchmarkPhaseSelectorE::Measure &&
+ if (BenchmarkPhaseSelector == BenchmarkPhaseSelectorE::Measure &&
!UseDummyPerfCounters) {
#ifndef HAVE_LIBPFM
ExitWithError(
@@ -504,7 +501,7 @@ void benchmarkMain() {
// Preliminary check to ensure features needed for requested
// benchmark mode are present on target CPU and/or OS.
- if (BenchmarkPhaseSelector >= BenchmarkPhaseSelectorE::Measure)
+ if (BenchmarkPhaseSelector == BenchmarkPhaseSelectorE::Measure)
ExitOnErr(State.getExegesisTarget().checkFeatureSupport());
if (ExecutionMode == BenchmarkRunner::ExecutionModeE::SubProcess &&
|
BaiXilin
pushed a commit
to BaiXilin/llvm-fix-vnni-instr-types
that referenced
this pull request
Jan 12, 2025
llvm#122371) …llvm#121991)" This reverts commit f8f8598. This breaks ARMv7 and s390x buildbot with the following message: ``` llvm-exegesis error: No available targets are compatible with triple "armv8l-unknown-linux-gnueabihf" FileCheck error: '<stdin>' is empty. FileCheck command line: /home/tcwg-buildbot/worker/clang-armv7-2stage/stage2/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv7-2stage/llvm/llvm/test/tools/llvm-exegesis/dry-run-measurement.test ```
mshockwave
added a commit
that referenced
this pull request
Jan 13, 2025
…121991)" (#122775) This relands f8f8598 Follow up on #122371: The problem here is a little subtle: when we dry-run the measurement phase, we create a LLJIT instance without actually executing the snippets. The key is, LLJIT has its own TargetMachine which uses triple designated by LLVM_TARGET_ARCH (which is default to host). On a machine that does not support Exegesis, the LLJIT would fail to create its TargetMachine because llvm-exegesis don't even register the host's target! Putting this test into any of the target-specific folder won't help, because it's about the host. And personally I don't really want to use `exegesis-can-execute-<arch>` for generic tests like this -- it's too strict as we don't actually need to execute the snippet. My solution here is creating another test feature which is added only when LLVM_TARGET_ARCH is supported by llvm-exegesis. This feature is something in between `<arch>-registered-target` and `exegesis-can-execute-<arch>`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…#121991)"
This reverts commit f8f8598.
This breaks ARMv7 and s390x buildbot with the following message:
Which I'm a little confused because I'm sure it's coming from
TargetSelect::lookupTargetbut...why isn't its own triple got registered?