Summary: | Support WebAssembly tail calls | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alessandro Pignotti <alessandro> | ||||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | REOPENED --- | ||||||||||||||||||
Severity: | Normal | CC: | carlo, chi187, commit-queue, cryze92, ews-watchlist, hypertree, justin_michaud, keith_miller, mark.lam, msaboff, saam, terrorjack, tom, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | Safari 13 | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 249492, 265262 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Alessandro Pignotti
2020-08-07 10:22:12 PDT
Created attachment 457113 [details]
Patch
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". 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 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 Updated the title as suggested Hmm, seems that EWS gets red. Can you take a look? Yes, I'm on it. Created attachment 458500 [details]
Patch
Created attachment 458515 [details]
with Changelogs
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. Created attachment 460584 [details]
Change returns offsets in wasm convention
Created attachment 460585 [details]
Implement wasm tail calls
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. Created attachment 460587 [details]
Implement wasm tail calls
Created attachment 460626 [details]
Implement wasm tail calls
Attempt #1 to fix arm build
@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. :) Pull request: https://github.com/WebKit/WebKit/pull/2065 *** Bug 242322 has been marked as a duplicate of this bug. *** Thanks! I joined Committed 258008@main (5ee0680f6669): <https://commits.webkit.org/258008@main> Reviewed commits have been landed. Closing PR #2065 and removing active labels. Re-opened since this is blocked by bug 249492 Why is this still open if blocker is indeed resolved? WASM in Safari still does not have tail calls. |