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

add riscv64 backend for cranelift. #4271

Merged
merged 208 commits into from
Sep 28, 2022
Merged

Conversation

yuyang-ok
Copy link
Contributor

@yuyang-ok yuyang-ok commented Jun 15, 2022

I am been trying to add riscv64 backend for cranelift these days.
right now I have pass all run test in filetests.

some features not implemented right now.
i128 mul div rem, all simd type and compare overflow.

some test need platform support.
like bitrev need qemu-riscv64 support bitmanip and zbkb extension (don't know how to enable it.).

@yuyang-ok
Copy link
Contributor Author

yuyang-ok commented Sep 25, 2022

@cfallin I think we are ready.
maybe some part not appropriate like https://github.com/yuyang-ok/wasmtime/blob/risc-v/crates/wasmtime/src/engine.rs#L493 .
maybe you can help me a little??

@a1phyr
Copy link
Contributor

a1phyr commented Sep 26, 2022

I think you could do the same that for s390x, i.e. calling getauxval by yourself.

You can find a RISC-V example in std_detect (the implementation of is_riscv64_feature_detected!) here.

@yuyang-ok
Copy link
Contributor Author

yuyang-ok commented Sep 26, 2022

I think you could do the same that for s390x, i.e. calling getauxval by yourself.

still look a little hard to use to me. look like need use C???

@yuyang-ok
Copy link
Contributor Author

@cfallin what do you think appropriate to solve is_riscv64_feature_detected???

@cfallin
Copy link
Member

cfallin commented Sep 27, 2022

@cfallin what do you think appropriate to solve is_riscv64_feature_detected???

If we want to wait for stabilization of the feature-detection macro in the standard library, I don't see anything wrong with assuming a baseline RISC-V ISA level for now (in other words, running with no features ever detected). Does this seem reasonable or would it result in important optimizations going missing?

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuyang-ok -- I went over the rest of the code and I think this looks generally fine. At least, passing all Wasm core tests indicates that this is of sufficient quality to merge, and can be refined and improved more in-tree.

I do have one minor comment below; and there's my answer to the getauxval question above. I'm not opposed to ignoring the getauxval issue for now (and simply not detecting features), if baseline RISC-V without extra ISA features is good enough to get started. And re: the below, a comment and factoring out the logic to a helper should be enough I think. Let me know once you've done this and then we can merge.

Thanks so much for the patience and hard work as we've reviewed this; I'm really excited to see it merge soon!

cranelift/filetests/src/test_run.rs Outdated Show resolved Hide resolved
@yuyang-ok
Copy link
Contributor Author

yuyang-ok commented Sep 27, 2022

Does this seem reasonable or would it result in important optimizations going missing?

popcnt,ctz,rev8,brev8 will fallback to loop implementation, seen reasonable to me, I think these instructions are used rare to me.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates look good to me -- thanks!

I'm going to go ahead and merge this now. Thanks again @yuyang-ok -- this was a huge amount of work.

More improvements are always welcome of course, from you and others -- anyone who wants to talk about further improvements to the RISC-V backend, please feel free to join our project meeting or file issues!

@cfallin cfallin merged commit cdecc85 into bytecodealliance:main Sep 28, 2022
@yuyang-ok
Copy link
Contributor Author

@cfallin ok,thanks. :-)

alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Sep 28, 2022
Our submodule was accidentally reverted to an older commit as part
of bytecodealliance#4271 and while it could be updated to as it was before I went ahead
and updated it to `main`.
alexcrichton added a commit that referenced this pull request Sep 28, 2022
* Update spec test repo

Our submodule was accidentally reverted to an older commit as part
of #4271 and while it could be updated to as it was before I went ahead
and updated it to `main`.

* Update ignore directives and test multi-memory

* Update riscv ignores
@rice7th
Copy link

rice7th commented Oct 8, 2022

@cfallin ok,thanks. :-)

Congratulations!

@martasp
Copy link

martasp commented Nov 12, 2022

How can I use this feature, is there documentation?
How can I set a target risc5 and produce Linux risc5 elf binary?
I would want to use it like that:

wasmtime exampleProgram.wasm --target risc5 

@archanox
Copy link

This work is for RISC-V not risc5.
You'll likely need to use riscv64gc as per the changes in this PR.

@martasp
Copy link

martasp commented Nov 12, 2022

I see that we have binaries riscv64gc here: https://github.com/bytecodealliance/wasmtime/releases/tag/v2.0.2 therefore It should probably work on riscv64 Linux. So I tried to install wasmtime in riscv64 Linux but without success.
Probably there is a difference between riscv64 and riscv64gc but I'm kinda new to these things, where can I learn it, any good resources?

[root@localhost ~]# curl https://wasmtime.dev/install.sh -sSf | bash
Error: Sorry! Wasmtime currently only provides pre-built binaries for x86_64 (Linux, macOS, Windows), aa
rch64 (Linux, macOS), and s390x (Linux).
 
[root@localhost ~]# uname
Linux
[root@localhost ~]# uname -p
riscv64

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 12, 2022

The script at https://wasmtime.dev/install.sh hasn't been updated yet for riscv64 support. You should probably directly download the binary from https://github.com/bytecodealliance/wasmtime/releases/tag/v2.0.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants