Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translate Wasm multi-value into cranelift IR #1049

Merged

Conversation

@fitzgen
Copy link
Member

fitzgen commented Sep 19, 2019

f? @sunfishcode

This is a work-in-progress.

Right now, it can translate the test WAT files into cranelift IR but the IR doesn't pass the verifier. I'd like to debug this further, but I'm in a little bit of a rut due to my unfamiliarity with this code base. As suggested on IRC, I'm trying to view the full IR, rather than just the snippet that gets printed with the verifier error, via clif-util -p wasm. Unfortunately, it looks like that command is still running the IR verifier, and is printing the same verifier error as cargo test -p cranelift-wasm does. I looked into clif-util's flags and didn't see anything about disabling the verifier. Any tips here?

Additionally, if you have any suggestions for the code itself, they would be most welcome. I have some "TODO FITZGEN" comments in there that pose some design/architecture questions for things I wasn't super sure about as well.

@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Sep 19, 2019

Unfortunately, it looks like that command is still running the IR verifier, and is printing the same verifier error as cargo test -p cranelift-wasm does.

For example:

$ cargo run --bin clif-util -- wasm -p wasmtests/multi-1.wat --target riscv32-unknown-linux-gnu
   Compiling cranelift-tools v0.42.0 (/home/fitzgen/cranelift)
    Finished dev [unoptimized + debuginfo] target(s) in 6.14s
     Running `target/debug/clif-util wasm -p wasmtests/multi-1.wat --target riscv32-unknown-linux-gnu`
function u0:0(i64, i64, i32 vmctx) -> i64, i64, i64 system_v {
                                ebb0(v0: i64, v1: i64, v2: i32):
@0032                               v8 = iconst.i64 1234
@0035                               jump ebb2(v1, v0, v8)
;~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
; error: inst1: mismatched argument count for `jump ebb2(v1, v0, v8)`: got 3, expected 2


                                ebb2(v6: i64, v7: i64):
@0036                               jump ebb1(v0, v6, v7)

                                ebb1(v3: i64, v4: i64, v5: i64):
@0036                               return v3, v4, v5
}

; 1 verifier error detected (see above). Compilation aborted.
@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Sep 19, 2019

Oh wait hm -- I think that is the full IR for this test case.

@sunfishcode

This comment has been minimized.

Copy link
Member

sunfishcode commented Sep 19, 2019

You can disable the verifier with --set enable_verifier=false, though yes, that is the complete IR for the function in question.

@fitzgen fitzgen force-pushed the fitzgen:translate-wasm-multi-value branch from 09071e5 to 9186027 Sep 23, 2019
@fitzgen fitzgen force-pushed the fitzgen:translate-wasm-multi-value branch from 9186027 to f9403b9 Sep 23, 2019
@fitzgen fitzgen changed the title WIP Translate wasm multi-value into cranelift IR Translate Wasm multi-value into cranelift IR Sep 23, 2019
@fitzgen fitzgen requested a review from sunfishcode Sep 23, 2019
@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Sep 23, 2019

Ok, I think this is ready for a fresh round of review!

@fitzgen fitzgen force-pushed the fitzgen:translate-wasm-multi-value branch 2 times, most recently from 26bd651 to 3969ca3 Sep 23, 2019
@fitzgen fitzgen force-pushed the fitzgen:translate-wasm-multi-value branch from 3969ca3 to 5f5e516 Sep 24, 2019
@fitzgen fitzgen requested a review from sunfishcode Sep 24, 2019
@fitzgen fitzgen force-pushed the fitzgen:translate-wasm-multi-value branch from 5f5e516 to fee18d8 Sep 24, 2019
@fitzgen fitzgen force-pushed the fitzgen:translate-wasm-multi-value branch 3 times, most recently from 2f8c876 to 70c3d45 Sep 26, 2019
Copy link
Member

sunfishcode left a comment

Looks good!

For historical interest, this is another thing which would have been nicer if wasm had had something like if_else ... end ... end and if ... end rather than if ... else ... end and if ... end, because we could then avoid pushing the extra values on the stack when there's no else.

@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Oct 1, 2019

Note: this isn't quite ready to land yet, I've found some bugs while running the spec test suite in wasmtime

@fitzgen fitzgen force-pushed the fitzgen:translate-wasm-multi-value branch from 70c3d45 to dccf8e5 Oct 1, 2019
@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Oct 1, 2019

Update: I have most of the multi-value spec tests passing in wasmtime. The failing tests fall into these buckets:

  • There is the expected failure for functions with many return values, where we only currently support returning as many values as can fit in registers. So we either need a way to disable certain functions within a spec test file (more of an issue for wasmtime) or we need to implement returning >N values via stack pointer (not sure yet if this is more of an embedder/environ responsibility or if cranelift itself needs to get involved). Opinions on which approach to take, @sunfishcode?

  • There is something funky going on with the factorial in multi-value test. It seems to infinite loop and I'm not 100% sure why yet. I'm trying to read the cranelift IR that we translate the wasm into, but I'm running into a couple snags. I can't seem to figure out how to look at the IR exactly as it comes out of cranelift-wasm, before any target-specific legalization passes or optimizations or anything of that sort. The x86-64-specific IR has a lot of noise in it, which is making it pretty hard to read. Also the local, direct function calls are getting turned into call_indirects when I use clif-util, which is pretty confusing, and I'm not sure whether it is also happening with wasmtime and running the spec tests or not.

    @sunfishcode any way to dump the pre-legalization/etc IR with clif-util?

@fitzgen fitzgen force-pushed the fitzgen:translate-wasm-multi-value branch from dccf8e5 to 4967ca3 Oct 1, 2019
@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Oct 2, 2019

There is something funky going on with the factorial in multi-value test. It seems to infinite loop and I'm not 100% sure why yet.

FWIW, I think I am not properly popping the initial loop params and then pushing the ebb's params. Will look into it tomorrow. The question about dumping the IR still stands though :)

@bnjbvr

This comment has been minimized.

Copy link
Member

bnjbvr commented Oct 2, 2019

clif-util compile -dtp ./file.wasm only does the translation (-t), prints resulting IR (-p) in stdout (-d). Is this sufficient for your needs?

This commit introduces initial support for multi-value Wasm. Wasm blocks and
calls can now take and return an arbitrary number of values.

The encoding for multi-value blocks means that we need to keep the contents of
the "Types" section around when translating function bodies. To do this, we
introduce a `WasmTypesMap` type that maps the type indices to their parameters
and returns, construct it when parsing the "Types" section, and shepherd it
through a bunch of functions and methods when translating function bodies.
@fitzgen fitzgen force-pushed the fitzgen:translate-wasm-multi-value branch from 4967ca3 to 58f6397 Oct 2, 2019
@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Oct 2, 2019

FWIW, I think I am not properly popping the initial loop params and then pushing the ebb's params.

Yep, this was it. This PR passes all the multi-value spec tests other than the ones that test more return values than we can fit in registers.

@fitzgen fitzgen merged commit bf47282 into bytecodealliance:master Oct 2, 2019
13 checks passed
13 checks passed
CraneStation.cranelift Build #refs_pull_1049_merge-2019-10-02.1 succeeded
Details
CraneStation.cranelift (Build mac) Build mac succeeded
Details
CraneStation.cranelift (Build windows) Build windows succeeded
Details
CraneStation.cranelift (Build_linux) Build_linux succeeded
Details
CraneStation.cranelift (Fuzz regression) Fuzz regression succeeded
Details
CraneStation.cranelift (Test linux-earliest) Test linux-earliest succeeded
Details
CraneStation.cranelift (Test mac-beta) Test mac-beta succeeded
Details
CraneStation.cranelift (Test mac-earliest) Test mac-earliest succeeded
Details
CraneStation.cranelift (Test mac-nightly) Test mac-nightly succeeded
Details
CraneStation.cranelift (Test mac-stable) Test mac-stable succeeded
Details
CraneStation.cranelift (Test windows-earliest) Test windows-earliest succeeded
Details
CraneStation.cranelift (docs) docs succeeded
Details
CraneStation.cranelift (rustfmt) rustfmt succeeded
Details
@fitzgen fitzgen deleted the fitzgen:translate-wasm-multi-value branch Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.