1641504 - Cranelift aarch64: support multi-values

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Opening this bug to put up a WIP patch to support multi-value in Baldrdash, based on Chris's PR: https://github.com/bytecodealliance/wasmtime/pull/1774

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1641504: Adapt to latest Cranelift API changes;

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Bug 1641504: Support multiple values in Cranelift aarch64; WIP

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Ah, actually we need to pass a proper ModuleTranslationState which contains function types, which will need to be duplicated.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Updated patch; there's now an error in Cranelift, which might be a preexisting one. To be investigated more!

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Note: to make it work, this is also needed on the wasmtime's side:

diff --git a/cranelift/wasm/src/state/module_state.rs b/cranelift/wasm/src/state/module_state.rs
index e99730585..52c650b5e 100644
--- a/cranelift/wasm/src/state/module_state.rs
+++ b/cranelift/wasm/src/state/module_state.rs
@@ -3,8 +3,7 @@ use cranelift_entity::PrimaryMap;
 use std::boxed::Box;
 
 /// Map of signatures to a function's parameter and return types.
-pub(crate) type WasmTypes =
-    PrimaryMap<SignatureIndex, (Box<[wasmparser::Type]>, Box<[wasmparser::Type]>)>;
+pub type WasmTypes = PrimaryMap<SignatureIndex, (Box<[wasmparser::Type]>, Box<[wasmparser::Type]>)>;
 
 /// Contains information decoded from the Wasm module that must be referenced
 /// during each Wasm function's translation.
@@ -28,4 +27,9 @@ impl ModuleTranslationState {
             wasm_types: PrimaryMap::new(),
         }
     }
+
+    /// Creates a new ModuleTranslationState with the given signature types.
+    pub fn with_sig_types(wasm_types: WasmTypes) -> Self {
+        Self { wasm_types }
+    }
 }

Also: this was an experiment to make it work, and should not land as is. There are a few back and forths between types here (from Spidermonkey's types to Cranelift's types to wasmparser types), and I think we should propose to rework the ModuleTranslationState (e.g. make it a trait) before doing this the proper way.

Comment 6

[Chris Fallin [:cfallin]]

Attachment: cranelift-table-env-api.patch

Also need this patch on the SpiderMonkey side.

Comment 7

[Chris Fallin [:cfallin]]

To briefly note here, the plan is to land multivalue support for aarch64 on the Cranelift side first (Wasmtime PR #1774), then version-bump Cranelift and make use of the multivalue support in one patch on the mozilla-central side. We have some new test failures in Spidermonkey-using-Cranelift, and these need multivalue support to resolve, so unfortunately the three issues (current CL failures, version bump of CL, and multivalue support) are intertwined.

Comment 8

[Chris Fallin [:cfallin]]

Attachment: Bug 1641504: Bump Cranelift to 4d5fdfcbba1a8f38002a4223d7a329fc795d0e9f. r=bbouvier

DO NOT COMMIT as-is: we need to get the above commit into mainline
Wasmtime first.

Comment 9

[Chris Fallin [:cfallin]]

Attachment: Bug 1641504: Adapt to Cranelift API changes. r=bbouvier

This is an updated version of Ben Bouvier's patch D77227.

Co-authored-by: Benjamin Bouvier <[e-mail]>

Depends on D78587

Comment 10

[Chris Fallin [:cfallin]]

Attachment: Bug 1641504: Wasm multi-value support using Cranelift backend. r=jseward

This is an adaptation of @bbouvier's patch D77228, updated to avoid
passing wasmparser types across the API boundary between SpiderMonkey
and Cranelift.

Co-authored-by: Benjamin Bouvier <[e-mail]>

Depends on D78588

Comment 11

[Chris Fallin [:cfallin]]

Just added a patch series that is an updated of Ben's patches together with a version-bump of Cranelift.

This depends on the Cranelift PR to be merged first; once that happens, I'll update D78587 (the vendoring patch) with the proper commit hash from bytecodealliance/wasmtime.

I've modified Ben's patch a bit to not pass wasmparser types over the API boundary; if there are better ways to do this then we can clean it up further, of course.

With these patches, we pass all Wasm tests except for the 3 interrupt-test failures noted in Bug 1641264.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dedeac296eaa
Bump Cranelift to e3d89c8a92a5fadedd75359b8485d23ac45ecf29. r=bbouvier
https://hg.mozilla.org/integration/autoland/rev/bf1919e75e65
Adapt to Cranelift API changes. r=bbouvier
https://hg.mozilla.org/integration/autoland/rev/95646dbd26a1
Wasm multi-value support using Cranelift backend. r=jseward

Comment 13

[Razvan Maries]

Backed out for build bustages.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2Cx64%2Copt%2Cvalgrind-linux64-valgrind%2Fopt%2C%28v%29&revision=95646dbd26a1d42d2cadfc8782608abe37418529&selectedTaskRun=b9EjwbiaRQS3zYP-129PoQ.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=305994046&repo=autoland&lineNumber=24829

Backout: https://hg.mozilla.org/integration/autoland/rev/0de2b561c2d857895ef02ae774b84e83f8368b77

Comment 14

[Chris Fallin [:cfallin]]

Argh, sorry about that: it seems that the build on CI uses an older version of Rust, and a few uses of matches!() (a newer feature) have crept into the Cranelift codebase. I'll fix it on the CL side and then revendor.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b4f55719649
Bump Cranelift to 4d5fdfcbba1a8f38002a4223d7a329fc795d0e9f. r=bbouvier
https://hg.mozilla.org/integration/autoland/rev/23fd1073665e
Adapt to Cranelift API changes. r=bbouvier
https://hg.mozilla.org/integration/autoland/rev/3a1b105a612c
Wasm multi-value support using Cranelift backend. r=jseward

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7b4f55719649
https://hg.mozilla.org/mozilla-central/rev/23fd1073665e
https://hg.mozilla.org/mozilla-central/rev/3a1b105a612c