[llvm-exegesis] Set stack pointer register after starting perf counter #72489
Conversation
Before this patch, in subprocess mode, llvm-exegesis setup the stack pointer register with the rest of the registers when it was requested by the user. This would cause a segfault when the instructions to start the perf counter ran as they use the stack to preserve the three registers needed to make the syscall. This patch moves the setup of the stack register to after the configuration of the perf counter to fix this issue so that we have a valid stack pointer for all the preceeding operations. Regression test added.
|
@llvm/pr-subscribers-tools-llvm-exegesis Author: Aiden Grossman (boomanaiden154) ChangesBefore this patch, in subprocess mode, llvm-exegesis setup the stack pointer register with the rest of the registers when it was requested by the user. This would cause a segfault when the instructions to start the perf counter ran as they use the stack to preserve the three registers needed to make the syscall. This patch moves the setup of the stack register to after the configuration of the perf counter to fix this issue so that we have a valid stack pointer for all the preceeding operations. Regression test added. This fixes #72193. Full diff: https://github.com/llvm/llvm-project/pull/72489.diff 4 Files Affected:
diff --git a/llvm/test/tools/llvm-exegesis/X86/latency/subprocess-rsp.s b/llvm/test/tools/llvm-exegesis/X86/latency/subprocess-rsp.s
new file mode 100644
index 000000000000000..2eac0a661a270ce
--- /dev/null
+++ b/llvm/test/tools/llvm-exegesis/X86/latency/subprocess-rsp.s
@@ -0,0 +1,22 @@
+# REQUIRES: exegesis-can-execute-x86_64
+
+# RUN: llvm-exegesis -mtriple=x86_64-unknown-unknown -mode=latency -snippets-file=%s -execution-mode=subprocess | FileCheck %s
+
+# See comment in ./subprocess-abnormal-exit-code.s on the transient
+# PTRACE_ATTACH failure.
+# ALLOW_RETRIES: 2
+
+# Check that we can set the value of RSP in subprocess mode without
+# segfaulting as we need to restore it after the rest of the setup is
+# complete to prevent loading from the stack where we set it instead
+# of where the stack actuall is.
+
+# LLVM-EXEGESIS-MEM-DEF test1 4096 2147483647
+# LLVM-EXEGESIS-MEM-MAP test1 1048576
+# LLVM-EXEGESIS-DEFREG RAX 100000
+# LLVM-EXEGESIS-DEFREG R14 100000
+# LLVM-EXEGESIS-DEFREG RSP 100000
+
+movq %r14, (%rax)
+
+# CHECK-NOT: error: 'The benchmarking subprocess sent unexpected signal: Segmentation fault'
diff --git a/llvm/tools/llvm-exegesis/lib/Assembler.cpp b/llvm/tools/llvm-exegesis/lib/Assembler.cpp
index 167fb6373377c28..43ab730eba90a98 100644
--- a/llvm/tools/llvm-exegesis/lib/Assembler.cpp
+++ b/llvm/tools/llvm-exegesis/lib/Assembler.cpp
@@ -60,6 +60,13 @@ static bool generateSnippetSetupCode(
BBF.addInstructions(ET.setStackRegisterToAuxMem());
}
for (const RegisterValue &RV : RegisterInitialValues) {
+ if (GenerateMemoryInstructions) {
+ // If we're generating memory instructions, don't load in the value for
+ // the register with the stack pointer as it will be used later to finish
+ // the setup.
+ if (RV.Register == ET.getStackRegister())
+ continue;
+ }
// Load a constant in the register.
const auto SetRegisterCode = ET.setRegTo(*MSI, RV.Register, RV.Value);
if (SetRegisterCode.empty())
@@ -70,6 +77,18 @@ static bool generateSnippetSetupCode(
#ifdef HAVE_LIBPFM
BBF.addInstructions(ET.configurePerfCounter(PERF_EVENT_IOC_RESET, true));
#endif // HAVE_LIBPFM
+ for (const RegisterValue &RV : RegisterInitialValues) {
+ // Load in the stack register now as we're done using it elsewhere
+ // and need to set the value in preparation for executing the
+ // snippet.
+ if (RV.Register != ET.getStackRegister())
+ continue;
+ const auto SetRegisterCode = ET.setRegTo(*MSI, RV.Register, RV.Value);
+ if (SetRegisterCode.empty())
+ IsSnippetSetupComplete = false;
+ BBF.addInstructions(SetRegisterCode);
+ break;
+ }
}
return IsSnippetSetupComplete;
}
diff --git a/llvm/tools/llvm-exegesis/lib/Target.h b/llvm/tools/llvm-exegesis/lib/Target.h
index 6de5b3c1065f1aa..b4fae7bff70f097 100644
--- a/llvm/tools/llvm-exegesis/lib/Target.h
+++ b/llvm/tools/llvm-exegesis/lib/Target.h
@@ -170,6 +170,12 @@ class ExegesisTarget {
"configurePerfCounter is not implemented on the current architecture");
}
+ // Returns the stack register for the platform.
+ virtual unsigned getStackRegister() const {
+ report_fatal_error(
+ "getStackRegister is not implemented on the current architecture");
+ }
+
// Gets the ABI dependent registers that are used to pass arguments in a
// function call.
virtual std::vector<unsigned> getArgumentRegisters() const {
diff --git a/llvm/tools/llvm-exegesis/lib/X86/Target.cpp b/llvm/tools/llvm-exegesis/lib/X86/Target.cpp
index ac99e98cc851f34..33a248110d8d63e 100644
--- a/llvm/tools/llvm-exegesis/lib/X86/Target.cpp
+++ b/llvm/tools/llvm-exegesis/lib/X86/Target.cpp
@@ -745,6 +745,8 @@ class ExegesisX86Target : public ExegesisTarget {
std::vector<MCInst> configurePerfCounter(long Request, bool SaveRegisters) const override;
+ unsigned getStackRegister() const override;
+
std::vector<unsigned> getArgumentRegisters() const override;
std::vector<unsigned> getRegistersNeedSaving() const override;
@@ -1211,6 +1213,8 @@ ExegesisX86Target::configurePerfCounter(long Request, bool SaveRegisters) const
return ConfigurePerfCounterCode;
}
+unsigned ExegesisX86Target::getStackRegister() const { return X86::RSP; }
+
std::vector<unsigned> ExegesisX86Target::getArgumentRegisters() const {
return {X86::RDI, X86::RSI};
}
|
boomanaiden154
changed the title
|
This probably isn't the cleanest way to do this. Very open to suggestions in that regard. It would probably be cleaner to not use the stack and move values directly between registers and memory, but that requires clobbering at least one register to hold a memory address as there is no move instruction that takes a 64 bit immediate as an address, so that isn't a viable solution. Maybe there's a solution that I'm missing though. Ready for reviewer comments either way though. |
|
I'm wondering whether we even want to support user-defined values for the stack pointer anyway. Is there a use case for this ? |
Yes. For the execution of arbitrary basic blocks with memory annotations for the construction of a datasets of BBs, there will definitely be BBs using the |
|
@legrosbuffle Can you take another look at this? It should be ready and I think there's a compelling case for supporting user-set stack pointer registers. |
…f counter (#72489)" This reverts commit 52b4b35. This commit was causing build failures on bots without libpfm installed as the CHECK requires were not set correctly. The test added should only run on machines with libpfm enabled due to the limitations currently imposed by the subprocess execution mode.
Before this patch, in subprocess mode, llvm-exegesis setup the stack pointer register with the rest of the registers when it was requested by the user. This would cause a segfault when the instructions to start the perf counter ran as they use the stack to preserve the three registers needed to make the syscall. This patch moves the setup of the stack register to after the configuration of the perf counter to fix this issue so that we have a valid stack pointer for all the preceeding operations.
Regression test added.
This fixes #72193.