[WebAssembly] Disallow tail calls with byval arguments

[tlively]

WebAssembly disallows tail calls with stack-allocated arguments because they tail calls leave no way to fix up the stack pointer after the call. WebAssembly also passes byval arguments on the stack, but we were not considering them when deciding whether a tail call should be allowed. Properly disallow tail calls in the presence of byval arguments and fix the existing test that should have caught this.

Fixes #124443.

[llvmbot]

@llvm/pr-subscribers-backend-webassembly

Author: Thomas Lively (tlively)

Changes

WebAssembly disallows tail calls with stack-allocated arguments because they tail calls leave no way to fix up the stack pointer after the call. WebAssembly also passes byval arguments on the stack, but we were not considering them when deciding whether a tail call should be allowed. Properly disallow tail calls in the presence of byval arguments and fix the existing test that should have caught this.

Fixes #124443.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp (+7)
• (modified) llvm/test/CodeGen/WebAssembly/tailcall.ll (+4-3)

diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
index 02db1b142a22b5..4cc06f057a5760 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
@@ -1211,6 +1211,13 @@ WebAssemblyTargetLowering::LowerCall(CallLoweringInfo &CLI,
         }
       }
     }
+    // Similarly, we cannot tail call with byval arguments.
+    for (unsigned I = 0; I < CLI.Outs.size(); ++I) {
+      const ISD::OutputArg &Out = CLI.Outs[I];
+      if (Out.Flags.isByVal() && Out.Flags.getByValSize() != 0)
+        NoTail(
+            "WebAssembly does not support tail calling with stack arguments");
+    }
   }
 
   SmallVectorImpl<ISD::InputArg> &Ins = CLI.Ins;
diff --git a/llvm/test/CodeGen/WebAssembly/tailcall.ll b/llvm/test/CodeGen/WebAssembly/tailcall.ll
index 84bd142462e37e..a1eeaf9f2e77e4 100644
--- a/llvm/test/CodeGen/WebAssembly/tailcall.ll
+++ b/llvm/test/CodeGen/WebAssembly/tailcall.ll
@@ -296,12 +296,13 @@ define i32 @mismatched_byval(ptr %x) {
 ; CHECK-NEXT:    global.set __stack_pointer, $pop7
 ; CHECK-NEXT:    i32.load $push0=, 0($0)
 ; CHECK-NEXT:    i32.store 12($1), $pop0
+; CHECK-NEXT:    i32.const $push5=, 12
+; CHECK-NEXT:    i32.add $push6=, $1, $pop5
+; CHECK-NEXT:    call $0=, quux, $pop6
 ; CHECK-NEXT:    i32.const $push3=, 16
 ; CHECK-NEXT:    i32.add $push4=, $1, $pop3
 ; CHECK-NEXT:    global.set __stack_pointer, $pop4
-; CHECK-NEXT:    i32.const $push5=, 12
-; CHECK-NEXT:    i32.add $push6=, $1, $pop5
-; CHECK-NEXT:    return_call quux, $pop6
+; CHECK-NEXT:    return $0
   %v = tail call i32 @quux(ptr byval(i32) %x)
   ret i32 %v
 }

[efriedma-quic]

A function with a byval argument should be able to tail-call another function with a byval argument in the same position. See 914a399 376d7b2 .

[dschuff]

This message can probably more specifically mention byval arguments.

[tlively]

I was assuming that "byval argument" is not going to mean much to most developers who might see this error message.

[dschuff]

It seems that if this were actually a "matched" byval (i.e. the caller had a byval param that was passed directly to the callee's byval param) then it could work, since the caller wouldn't need to allocate space on its own stack for the byval.
I'm not sure it would be worth implementing that though.

[tlively]

Thanks for the references, @efriedma-quic. I don't quite understand the thing about copying via a temporary slot, but I don't think it applies to WebAssembly because we always copy to a fresh stack slot. My mental model for how this works may be wrong, though.

[efriedma-quic]

Aren't byval arguments passed via the explicit stack pointer? I think you have the same issues you'd have with a conventional architecture.

[tlively]

Probably, but I don't understand those issues :) Can you help explain what bad situation the double copy avoids?

I've also added some tests for edge cases we would have to handle if we tried to do the argument forwarding optimization. That optimization seems more complex than I had initially hoped, so I'd rather not tackle it in this PR.

[efriedma-quic]

Suppose you have a function like void g(S, S); void f(S a, S b) { return g(b, a); }. To tail-call, you have to essentially swap a and b. You use a temporary to avoid overwriting the argument before you copy it.