1657802 - Support new WebAssembly ABI in Cranelift backend

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Chris Fallin [:cfallin]]

The WebAssembly ABI is changing in Bug 1639153. Because we're planning to use Cranelift as a Wasm backend on AArch64 (ARM64), we need to modify the SpiderMonkey ABI implementation in Cranelift to support the new ABI.

Comment 1

[Lars T Hansen [:lth]]

In terms of priority, it's OK to wait on this until the ABI changes are stable on all the other platforms and compilers.

Comment 2

[Dmitry Bezhetskov]

Hi Chris, thanks you a lot for the intention to help!

I think we can change cranelift ABI because all other platforms are ready:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=abe6b765740c13659629fa1dcf35a7d626c64feb.
It is the last PR in the queue where we finally remove Frame::tls.

The queue starts here: https://phabricator.services.mozilla.com/D82881.
In short, in cranelift we just need to allocate 2 stack slots at call sites and fill them with caller tls and callee tls.

Comment 3

[Chris Fallin [:cfallin]]

:dbezhetskov, indeed, I can help out. I spent some time trying to understand how this works in the current Cranelift bindings today. I'm not entirely sure if Wasm-on-Cranelift-in-SpiderMonkey actually uses the TLS register; it has a notion of a "pinned register" but the Baldrdash code seems to imply this actually is used for the heap base (?). (Sorry, almost all of my Cranelift backend work has been independent of the snippets that the SpiderMonkey integration inserts, so I don't actually know how this works very well.)

I'll read more code tomorrow and try to get a better idea of what is actually required. It might need some API adjustments in Cranelift's notion of the "environment" so that the ABI code and the embedding-specific code can cooperate to stuff these stack slots.

Comment 4

[Dmitry Bezhetskov]

:cfallin, right, cranelift should use TLS register because they want to reload heap base, for example after calling the memory.grow.
Ion and Baseline load heap base like that:

loadPtr(Address(WasmTlsReg, offsetof(wasm::TlsData, memoryBase)), HeapReg); 

For more info you might want to read this https://docs.google.com/document/d/1oi6ROZJuE-hBb21Tgq-XcUegPktecF52gADzBJcYL34/edit#heading=h.urt5rn1n8340

Comment 5

[Chris Fallin [:cfallin]]

Attachment: Bug 1657802, part 1 of 2: Vendor Cranelift d04a84b53da3fd6e82e0ffdefe76a85cea814907. PRIVATE BRANCH DO NOT LAND. r=bbouvier

Comment 6

[Chris Fallin [:cfallin]]

Attachment: Bug 1657802: Adapt Cranelift-based Wasm to use ABI-2020. r=bbouvier

Depends on D91080

Comment 7

[Chris Fallin [:cfallin]]

I've gotten this to work for some tests but a bunch of tests still fail. Here's the branch of Cranelift that was vendored in the first patch above:

https://github.com/cfallin/wasmtime/tree/baldrdash-2020

I've defined a new ABI at the Cranelift level so that we don't have to tie the transitions in two different repos together (otherwise e.g. you might be unable to vendor newer Cranelift with any other needed changes if this rolls back). The changes on the Cranelift side are necessary because the CLIF (Cranelift IR) alone can't be generated in a way to create a different stack frame.

My understanding from that document was that basically we need to push caller-TLS and callee-TLS before every call -- this handles the caller side; the callee side is mostly already handled, I think, because Baldrdash inserts a Cranelift-generated function body in between a SpiderMonkey-generated prologue and epilogue.

I'll keep debugging this further but maybe :bbouvier can see something obvious I'm missing here?

Comment 8

[Lars T Hansen [:lth]]

FWIW, the ion/baseline work is not fully baked yet, some landed but got backed out for creating instability.

Comment 9

[Chris Fallin [:cfallin]]

This now seems to pass all jit-tests under wasm/. The associated Cranelift PR is #2223.

Also clearing n-i from Ben since I am no longer perplexed and confused :-)

Comment 10

[Chris Fallin [:cfallin]]

With latest changes, this passes jit-tests with Cranelift on x64 as well; testing on x64 exposed an issue w.r.t. restoring the TLS register.

So to summarize, this patch stack (applied on top of parent patch stack from :dbezhetskov) passes all jit-tests with --wasm-compiler=cranelift --no-wasm-simd on x86-64, and with --wasm-compiler=cranelift on aarch64 (the latter tested with a simulator build).

I think maybe the best thing to do, once the Cranelift PR is merged, would be to vendor this in separately (patch 1 of 2 here), then have :dbezhetskov adopt the SM-side change (patch 2 of 2 here) to land atomically with the rest of the patch stack. Does this seem reasonable?

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:dbezhetskov, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 12

[Dmitry Bezhetskov]

I landed the copy of Chris's patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1639153, so we are done here.