wasmtime / Issue #1605 Stub of a new isel backend for x64 · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / Issue #1605 Stub of a new isel backend for x64

Wasmtime GitHub notifications bot (Apr 27 2020 at 18:16):

github-actions[bot] commented on Issue #1605:

Subscribe to Label Action

cc @bnjbvr

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:meta"

Thus the following users have been cc'd because of the following labels:

bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Apr 28 2020 at 07:04):

julian-seward1 commented on Issue #1605:

Overall this looks fine to me. My only real request is: I'd prefer that the instruction-fragment definitions (args.rs) not be split off from the main instruction definitions. I found this something of a nuisance for the arm64 version, having to flip back and forth between two source files when looking at the overall structure of a new instruction. Also, the win is less here -- x64 has fewer argument-fragment kinds than arm64.

Wasmtime GitHub notifications bot (Apr 28 2020 at 10:43):

bnjbvr commented on Issue #1605:

My only real request is: I'd prefer that the instruction-fragment definitions (args.rs) not be split off from the main instruction definitions.

I did quite like the separation of it, since the inst/mod.rs file then only contains the overall Inst enum and associated helpers (more needs to be moved from inst/mod to inst/args). Eventually it'll even out the sizes of these files to around 600 or 700 lines each, which is nice too. It also cleanly separates concepts (instructions vs operands), and blackboxes the operands (which are unlikely to change much over time, compared with the instructions for which we might add more variants more frequently). Navigating between files doesn't seem like a strong annoyance in this case, since only two are involved (and LSP makes it really easy to jump-to-definitions, I'd expect text editors have then a feature to jump back to the previous position). Overall, it seems better to keep the two files separated.

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:58):

abrown commented on Issue #1605:

cc @jlb6740

Wasmtime GitHub notifications bot (Apr 28 2020 at 17:02):

abrown edited a comment on Issue #1605:

cc @jlb6740 @joshtriplett

Wasmtime GitHub notifications bot (Apr 30 2020 at 01:29):

jlb6740 commented on Issue #1605:

or add --set use_new_backend and this will use the new isel backend. (Expect panics, unimplemented errors, etc.)

Hi @bnjbvr. Do you think any example should show some signs of life here? I tried a simple module that adds to numbers with the current isel and got what I assume is expected results but running through the new isel there were no error messages or panics .. it just hung. Is this expected?

Wasmtime GitHub notifications bot (Apr 30 2020 at 14:17):

bnjbvr commented on Issue #1605:

Hi @bnjbvr. Do you think any example should show some signs of life here? I tried a simple module that adds to numbers with the current isel and got what I assume is expected results but running through the new isel there were no error messages or panics .. it just hung. Is this expected?

It's likely I went through compilation, but not actually running the code in wasmtime (since there seems to be no way to pass Cranelift CLI args through wasmtime runner).

I have used the following simple wasm module:
    (module
        (func (export "add") (param i32) (result i32)
            get_local 0
            i32.const 42
            i32.add
        )
compiled to wasm binary, and then, from Cranelift's directory (so as to use clif-util), ran the following:
cargo run -- wasm --target x86_64 --set use_new_backend=1 -dvTD /tmp/a.wasm # shows assembly generated with new backend
cargo run -- wasm --target x86_64 -dvTD /tmp/a.wasm # shows assembly generated with current backend

Wasmtime GitHub notifications bot (Apr 30 2020 at 15:51):

bnjbvr commented on Issue #1605:

Thanks all for the first round of comments! Many things we could do later on so as to improve this code (especially with respect to naming, and respecting Rust standards in general).

I just checked, and the simple module from the above comment works in Spidermonkey (with a few tweaks on the Spidermonkey side to make it use the new backend), which is a good sign.

Wasmtime GitHub notifications bot (Apr 30 2020 at 16:33):

jlb6740 commented on Issue #1605:

compiled to wasm binary, and then, from Cranelift's directory (so as to use clif-util), ran the following:

cargo run -- wasm --target x86_64 --set use_new_backend=1 -dvTD /tmp/a.wasm # shows assembly generated with new backend cargo run -- wasm --target x86_64 -dvTD /tmp/a.wasm # shows assembly generated with current backend

Ahh thanks ... this worked. The difference was the ordering of the parameters.

This works:

cargo run -- wasm --target x86_64 --set use_new_backend=1 -dvTD /tmp/a.wasm

While this hung:

cargo run -- wasm --target x86_64 -dvTD --set use_new_backend=1 /tmp/a.wasm

Wasmtime GitHub notifications bot (May 02 2020 at 04:44):

whitequark commented on Issue #1605:

What's the plan for supporting x86_32? The old backend was generic over the word size; this one doesn't seem to be.

Wasmtime GitHub notifications bot (May 02 2020 at 04:46):

whitequark edited a comment on Issue #1605:

What's the plan for supporting x86_32? The old backend was generic over the word size; this one doesn't seem to be. Does the new approach allow to make such a generic backend, or is it necessary to duplicate all the shared code?

Wasmtime GitHub notifications bot (May 05 2020 at 12:41):

bnjbvr commented on Issue #1605:

What's the plan for supporting x86_32? The old backend was generic over the word size; this one doesn't seem to be. Does the new approach allow to make such a generic backend, or is it necessary to duplicate all the shared code?

This question is still open for design. In my opinion, if it doesn't imply a huge cost on the x64 backend in terms of maintainability and readability, we should try to share and reuse as much as we can.

Wasmtime GitHub notifications bot (May 05 2020 at 14:01):

bnjbvr commented on Issue #1605:

Thanks for the reviews and new comments again! Will merge if CI passes.

Last updated: Nov 22 2024 at 16:03 UTC