Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WebAssembly] Define local sp if llvm.stacksave is used #68133

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

kateinoigakukun
Copy link
Member

Usually llvm.stacksave/stackrestore are used together with alloca but they can appear without it (e.g. alloca can be optimized away). WebAssembly's function local physical user sp register, which is referenced by llvm.stacksave is created while frame lowering and replaced with virtual register.
However the sp register was not created when llvm.stacksave is used without alloca, and it led MIR verification failure about use-before-def of sp virtual register.

Resolves #62235

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 3, 2023

@llvm/pr-subscribers-backend-webassembly

Changes

Usually llvm.stacksave/stackrestore are used together with alloca but they can appear without it (e.g. alloca can be optimized away). WebAssembly's function local physical user sp register, which is referenced by llvm.stacksave is created while frame lowering and replaced with virtual register.
However the sp register was not created when llvm.stacksave is used without alloca, and it led MIR verification failure about use-before-def of sp virtual register.

Resolves #62235


Full diff: https://github.com/llvm/llvm-project/pull/68133.diff

2 Files Affected:

  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp (+8-1)
  • (modified) llvm/test/CodeGen/WebAssembly/userstack.ll (+12)
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
index e60f1397b993355..d26234e7e71d1d4 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp
@@ -147,7 +147,14 @@ bool WebAssemblyFrameLowering::needsPrologForEH(
 /// Unlike a machine stack pointer, the wasm user stack pointer is a global
 /// variable, so it is loaded into a register in the prolog.
 bool WebAssemblyFrameLowering::needsSP(const MachineFunction &MF) const {
-  return needsSPForLocalFrame(MF) || needsPrologForEH(MF);
+  auto &MRI = MF.getRegInfo();
+  // llvm.stacksave can explicitly read SP register and it can appear without
+  // dynamic alloca.
+  bool hasExplicitSPUse = any_of(MRI.use_operands(getSPReg(MF)), [](MachineOperand &MO) {
+    return !MO.isImplicit();
+  });
+
+  return needsSPForLocalFrame(MF) || needsPrologForEH(MF) || hasExplicitSPUse;
 }
 
 /// Returns true if the local user-space stack pointer needs to be written back
diff --git a/llvm/test/CodeGen/WebAssembly/userstack.ll b/llvm/test/CodeGen/WebAssembly/userstack.ll
index b92946d1a6af7f2..ea0fd49503380d6 100644
--- a/llvm/test/CodeGen/WebAssembly/userstack.ll
+++ b/llvm/test/CodeGen/WebAssembly/userstack.ll
@@ -550,6 +550,18 @@ define void @llvm_stack_builtins(i32 %alloc) noredzone {
  ret void
 }
 
+; Use of stacksave requires local SP definition even without dymamic alloca.
+; CHECK-LABEL: llvm_stacksave_noalloca:
+define void @llvm_stacksave_noalloca() noredzone {
+ ; CHECK: global.get $push[[L11:.+]]=, __stack_pointer{{$}}
+ %stack = call i8* @llvm.stacksave()
+
+ ; CHECK-NEXT: call use_i8_star, $pop[[L11:.+]]
+ call void @use_i8_star(i8* %stack)
+
+ ret void
+}
+
 ; Not actually using the alloca'd variables exposed an issue with register
 ; stackification, where copying the stack pointer into the frame pointer was
 ; moved after the stack pointer was updated for the dynamic alloca.

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Member

@dschuff dschuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

llvm/lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp Outdated Show resolved Hide resolved
Usually `llvm.stacksave/stackrestore` are used together with `alloca`
but they can appear without it (e.g. `alloca` can be optimized away).
WebAssembly's function local physical user sp register, which is
referenced by `llvm.stacksave` is created while frame lowering and
replaced with virtual register. However the sp register was not created
when `llvm.stacksave` is used without `alloca`, and it led
MIR verification failure about use-before-def of sp virtual register.
@dschuff dschuff merged commit da0ca5d into llvm:main Oct 3, 2023
2 of 3 checks passed
@kateinoigakukun
Copy link
Member Author

Thank you for reviewing quickly!

@kateinoigakukun kateinoigakukun deleted the yt/wasm-stacksave-dom-upstream branch October 3, 2023 21:53
kateinoigakukun added a commit to swiftlang/llvm-project that referenced this pull request Oct 9, 2023
Usually `llvm.stacksave/stackrestore` are used together with `alloca`
but they can appear without it (e.g. `alloca` can be optimized away).
WebAssembly's function local physical user sp register, which is
referenced by `llvm.stacksave` is created while frame lowering and
replaced with virtual register.
However the sp register was not created when `llvm.stacksave` is used
without `alloca`, and it led MIR verification failure about
use-before-def of sp virtual register.

Resolves llvm#62235
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[WebAssembly] stacksave leads machine code verification failure after wasm-replace-phys-regs
3 participants