Experiment: Enable multi-value #1459
Conversation
but `drun` does not support it out of box.
happens to be the version integrated into `drun`
|
Ugh, we’d have to switch back to a fork of the |
|
Maybe there is a benefit in not trying to use any not-yet-fully-approved wasm featues. Because image we support more than one (say, multi-value and bulk memory). Then we’d have to maintain our own merge of the Or we stop using the ocaml wasm library, for these reasons? It’s a reference implementation after all, and not necessarily a production ready, convenient-to-use, new features avaiable behind feature flags library… But it’s certainly convenient sometimes to use the |
|
What's the status on tail calls? I think that's actually a more important feature for us because of our cps conversion.... |
|
Also some relevant more general discussion: |
|
Multi-value is fully approved and merged into the official spec at this point. It's also in the wasm interpreter. I merely didn't think of creating a new opam package. Will do today. |
|
Thanks! I missed that it had been merged. Also, the README of https://github.com/WebAssembly/multi-value/ maybe ought to indicate that. |
|
Good point. Added a note to the README. |
|
@crusso, tail calls are still at stage 3, awaiting a second implementation besides V8. |
will help with #1459 Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
|
This needs a rebase now, to reduce the diffs. Also note #1467, which is in the works. |
|
Updated (I tend to avoid rebase these days) |
ghost
assigned
|
Nice, |
inspired by the work in #1459 I figured it would be nice if Hydra runs the tests in `tests/run-drun` separately with `drun` and `ic-ref-run`. This way * We can see from the status list already if only one of the two break (e.g. after bumping dependencies or changing Wasm featurs) * We do less rebuilds (e.g. after bumping dependencies) * We have higher nix-level parallelism (relevant as `tests.run-drun` is one of the longest running tests) Thanks to #1216 this only needs a small change to `default.nix`.
* nix, tests: Split run-drun inspired by the work in #1459 I figured it would be nice if Hydra runs the tests in `tests/run-drun` separately with `drun` and `ic-ref-run`. This way * We can see from the status list already if only one of the two break (e.g. after bumping dependencies or changing Wasm featurs) * We do less rebuilds (e.g. after bumping dependencies) * We have higher nix-level parallelism (relevant as `tests.run-drun` is one of the longest running tests) Thanks to #1216 this only needs a small change to `default.nix`. * nix: Simplify name handling for test derivations Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
|
I was curious if recent updates to |
|
I suspect the problem is parity-wasm, which is used for instrumentation. See paritytech/parity-wasm#292 |
|
Other than @osa1's contribution of trunc_sat instructions, the parity impl seems to have made practically no progress over the last two years. That's rather concerning for a dependency. :( |
|
The base tests fall over this code which I introduced in 2ad8eed, three days after I branched off this branch. This will require a bit of refactoring. Thinking out loud: With multi-value returns, we need to produce a incidentially, this |
|
Fixed that |
These Wasm nodes need to be annotated with their return type, and if the return type is more than nullary or unary, then this type needs to be defined in the Wasm module type table. This refactors the story around `if_` etc. to allow that: `E.if_ env tys thn else` will register the types as needed But in many places we know statically that there is only zero or one return type. For them, `G.if0` and `G.if1` can be used for cleaner code and to emphasize their “purity”. Extracted from #1459, but I’d like to have it on master to prevent merge conflicts in #1459 (which is a long-running branch since May 2020…)
|
BTW, Claudio, do you (internally) already have canisters that use this? Becaues “multi-value compilation” is still an open issue in the Rust repo: rust-lang/rust#73755. It seems dangerous to turn on this feature if it isn’t fully supported by upstream rust already |
nomeata
force-pushed
the
joachim/multi-value
branch
from
5ffe568 to
5171f7a
Compare
These Wasm nodes need to be annotated with their return type, and if the return type is more than nullary or unary, then this type needs to be defined in the Wasm module type table. This refactors the story around `if_` etc. to allow that: `E.if_ env tys thn else` will register the types as needed But in many places we know statically that there is only zero or one return type. For them, `G.if0` and `G.if1` can be used for cleaner code and to emphasize their “purity”. In a similar vein, `G.loop0` is introduced; since our loops never produce values on the stack, there is no `loop_` with multi-value result. Extracted from #1459, but I’d like to have it on master to prevent merge conflicts in #1459 (which is a long-running branch since May 2020…)
No, the only reason I was asking about multi-value is that I've been asked to support the 128 bit cycles API which appears to use multi-value returns for System functions. I assume the replica has some tests, but perhaps those were hand-crafted wasm, not rust compiled. |
nomeata
force-pushed
the
joachim/multi-value
branch
from
5171f7a to
1b4a4f7
Compare
|
Gave it another shot. Fun fact: The fact that this only shows up in |
So far, the IC does not support wasm using multi-value returns from functions and blocks (see #1459). The backend was originally written with that feature in mind, and later we added a work-around (module `FakeMultiVal`). For functions we work around it by storing multiple values in fixed globals, and reading them after the call. No allocations needed. For blocks with multiple values, however, we allocate a tuple (array) on the heap, write the values there, and read them later. This adds unnecessary heap churn and fills up memory faster. So this PR uses the mechanism from the functions also for blocks. Goes well with #3556
So far, the IC does not support wasm using multi-value returns from functions and blocks (see #1459). The backend was originally written with that feature in mind, and later we added a work-around (module `FakeMultiVal`). For functions we work around it by storing multiple values in fixed globals, and reading them after the call. No allocations needed. For blocks with multiple values, however, we allocate a tuple (array) on the heap, write the values there, and read them later. This adds unnecessary heap churn and fills up memory faster. So this PR uses the mechanism from the functions also for blocks. Goes well with #3556 Here is a (particularly bad) example: In ```motoko func multi_value_blocks(n : Nat) : (Nat, Nat) { return ( if (n == 0) { returns_tuple() } else { if (n == 1) { returns_tuple() } else { returns_tuple() } } ); }; ``` Now no allocation happens (and all the reads and writes to the global variables are eliminated due to #3556): ```diff (func $multi_value_blocks (type 1) (param $clos i32) (param $n i32) local.get $n i32.const 0 call $B_eq - if (result i32) ;; label = @1 + if ;; label = @1 i32.const 0 call $returns_tuple - global.get 4 - global.get 5 - call $to_2_tuple else local.get $n i32.const 2 call $B_eq - if (result i32) ;; label = @2 + if ;; label = @2 i32.const 0 call $returns_tuple - global.get 4 - global.get 5 - call $to_2_tuple else i32.const 0 call $returns_tuple - global.get 4 - global.get 5 - call $to_2_tuple end end - call $from_2_tuple return) ```
|
The Haskell to WebAssembly story hasn't enabled multi-value yet due to llvm/llvm-project#59095. Worth keeping an eye on. |
|
Just doing some repo cleanup for better focussing on active work: |
drun now uses wasmtime-0.15, which supports multi-value.
--enable-multi-valueto itwinter(dfinity-side-projects/winter@9f9b702)winterandic-ref(Bump ic-ref #1462)dfinityenable multi-value inparity_wasmparser: https://github.com/dfinity-lab/dfinity/pull/5321dfinityenable multi-value in wasmtime: https://github.com/dfinity-lab/dfinity/pull/5324parity_wasmparser support it upstream: Multiple func results paritytech/parity-wasm#288wasmv1.1.1 (Bump wasm to opam-1.1.1 #1467)We can test all that on branches. Likely, though, allowing multi-value needs to
be part of a Public Spec release, so this can’t be the default for a while.