Bug 215275 - Support WebAssembly tail calls
Summary: Support WebAssembly tail calls
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari 13
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 242322 (view as bug list)
Depends on: 249492 265262
Blocks:
  Show dependency treegraph
 
Reported: 2020-08-07 10:22 PDT by Alessandro Pignotti
Modified: 2023-11-22 12:12 PST (History)
16 users (show)

See Also:


Attachments
Patch (52.72 KB, patch)
2022-04-08 12:55 PDT, Tom Tartarin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (1.05 MB, patch)
2022-04-28 01:27 PDT, Tom Tartarin
no flags Details | Formatted Diff | Diff
with Changelogs (1.09 MB, patch)
2022-04-28 05:02 PDT, Tom Tartarin
no flags Details | Formatted Diff | Diff
Change returns offsets in wasm convention (12.19 KB, patch)
2022-06-30 09:16 PDT, Tom Tartarin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Implement wasm tail calls (1.13 MB, patch)
2022-06-30 09:16 PDT, Tom Tartarin
no flags Details | Formatted Diff | Diff
Implement wasm tail calls (1.14 MB, patch)
2022-06-30 09:37 PDT, Tom Tartarin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Implement wasm tail calls (1.14 MB, patch)
2022-07-01 15:22 PDT, Tom Tartarin
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alessandro Pignotti 2020-08-07 10:22:12 PDT
Hi,

we have a use case that requires WebAssembly tail calls. You can find more information in this thread: https://github.com/WebAssembly/tail-call/issues/12

From what I can see by inspecting the repo, no work has been done so far to support WebAssembly tail calls in JavaScriptCore/Safari. I want to explicitly ask if there are plans to implement this feature and what is the expected timeline, if any.

Regards,
Alessandro
Comment 1 Radar WebKit Bug Importer 2020-08-14 10:23:23 PDT
<rdar://problem/67081070>
Comment 2 Tom Tartarin 2022-04-08 12:55:19 PDT
Created attachment 457113 [details]
Patch
Comment 3 EWS Watchlist 2022-04-08 12:56:59 PDT
This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".
Comment 4 Tom Tartarin 2022-04-08 12:59:17 PDT
As a follow up:

This is a WIP patch to gather some feedback before potentially going further.

Known limitations:
- B3 and AIR are stubbed
- probably some portability issues (only tested on x86_64)
- tests could be more extensive (multiple return values, recursive calls etc..)
- some thinking could be put into formatting the code in slowPathForWasmReturnCall

From the caller's point of view, this consists of reusing the previous frame with the callee's stack arguments/return values.


Right now, a slowPathForWasmTailCall is defined in WebAssembly.asm and will be only invoked for tail variants of call and call_indirect.
Conceptually it is a merge between slowPathForWasmCall and prepareForTailCall, that deals with passing arguments via registers.
The numberOfCallerStackArguments is also encoded in the corresponding bytecode.


Beside formatting and naming in the patch I'd like to discuss the viability of the implementation in the interpreter. In the context of LLIntPlan I didn't find a satisfactory way to JIT a specific entry thunk, and adding a dynamic check ( similar to what happens in JS) made less sense than creating a slowPath for tail variant of calls in the interpreter.

I also included some tests that cover basic cases.

For reference:
https://github.com/WebAssembly/tail-call
Comment 5 Yusuke Suzuki 2022-04-08 12:59:49 PDT
Comment on attachment 457113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457113&action=review

Cool, will look later today.

> Source/JavaScriptCore/ChangeLog:3
> +        Support for WebAssembly tail calls

Let's rename bug and patch's title to say, Support WebAssembly tail calls in LLInt tier
Comment 6 Alessandro Pignotti 2022-04-08 14:50:13 PDT
Updated the title as suggested
Comment 7 Yusuke Suzuki 2022-04-12 21:00:32 PDT
Hmm, seems that EWS gets red. Can you take a look?
Comment 8 Tom Tartarin 2022-04-12 23:28:18 PDT
Yes, I'm on it.
Comment 9 Tom Tartarin 2022-04-28 01:27:01 PDT
Created attachment 458500 [details]
Patch
Comment 10 Tom Tartarin 2022-04-28 05:02:40 PDT
Created attachment 458515 [details]
with Changelogs
Comment 11 Tom Tartarin 2022-05-01 23:55:03 PDT
To my knowledge, the limitations mentioned in comment 4 have been addressed. B3 and Air still redirect to a normal call as the bug focuses on the LLInt tier for now.
Comment 12 Tom Tartarin 2022-06-30 09:16:10 PDT
Created attachment 460584 [details]
Change returns offsets in wasm convention
Comment 13 Tom Tartarin 2022-06-30 09:16:59 PDT
Created attachment 460585 [details]
Implement wasm tail calls
Comment 14 Tom Tartarin 2022-06-30 09:25:27 PDT
I went on and implemented it in the other tiers, hence we should change the bug title.

I think the convention would need to change the locations of the stack return values if we were to follow the reasoning of unconditionally reusing the previous frame when tail-calling: 
With the current convention, the callee stack values are written from stackPointer + headerSize towards framePointer (caller's POV). When a frame is reused, the stack size for argument/returns can grow depending on the callee, which means that when at the return location of the original frame, given that the stack pointer is restored, the caller might not found the values at the right offsets, or they may even be lost.
In the first patch, we always align the stack size (max(stackArgs, stackReturns)) in our calculation, and start writing the return values at offset header + stackSize - numberOfReturnValues (still caller's POV). This way, return values are at the same locations, even if the frame is recycled in between.
In the generators, I am not sure how much refactoring is allowed since most of the code for ReturnCall is similar to Call.
Comment 15 Tom Tartarin 2022-06-30 09:37:46 PDT
Created attachment 460587 [details]
Implement wasm tail calls
Comment 16 Tom Tartarin 2022-07-01 15:22:06 PDT
Created attachment 460626 [details]
Implement wasm tail calls

Attempt #1 to fix arm build
Comment 17 Yusuke Suzuki 2022-07-02 15:52:21 PDT
@Tom BTW, WebKit dev has slack workspace for discussions, where anyone can join.
https://webkit.org/getting-started/#staying-in-touch
And there are #jsc channel, where JSC developers are talking about things.
So, you can join, request reviews, and discuss the design etc. :)
Comment 18 Tom Tartarin 2022-07-04 13:33:54 PDT
Pull request: https://github.com/WebKit/WebKit/pull/2065
Comment 19 Tom Tartarin 2022-07-04 13:38:02 PDT
*** Bug 242322 has been marked as a duplicate of this bug. ***
Comment 20 Tom Tartarin 2022-07-04 13:41:19 PDT
Thanks! I joined
Comment 21 EWS 2022-12-16 11:29:34 PST
Committed 258008@main (5ee0680f6669): <https://commits.webkit.org/258008@main>

Reviewed commits have been landed. Closing PR #2065 and removing active labels.
Comment 22 WebKit Commit Bot 2022-12-16 11:50:44 PST
Re-opened since this is blocked by bug 249492
Comment 23 Sanjay Kumar 2023-09-13 18:18:08 PDT
Why is this still open if blocker is indeed resolved? WASM in Safari still does not have tail calls.