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>
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.
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 overallInst
enum and associated helpers (more needs to be moved frominst/mod
toinst/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.
abrown commented on Issue #1605:
cc @jlb6740
abrown edited a comment on Issue #1605:
cc @jlb6740 @joshtriplett
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?
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
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.
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
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.
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?
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.
bnjbvr commented on Issue #1605:
Thanks for the reviews and new comments again! Will merge if CI passes.
Last updated: Dec 23 2024 at 12:05 UTC