-
Notifications
You must be signed in to change notification settings - Fork 632
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
Experimental aarch64 support #6200
Conversation
runtime/near-vm-runner/Cargo.toml
Outdated
@@ -52,6 +38,22 @@ threadpool = "1.8.1" | |||
pwasm-utils_12 = { package = "pwasm-utils", version = "0.12" } | |||
parity-wasm_41 = { package = "parity-wasm", version = "0.41" } | |||
|
|||
[target.'cfg(not(target = "aarch64"))'.dependencies] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nagisa curious, what you think is the right approach here?
- as is, the caller just has to avoid depending on wasmer on non-x86_64 platforms
- make wasmer compiler for any platform, but error out when trying to compile/run stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that wasmer2 just landed aarch64 support for singlepass, I suspect that we might want to undo this eventually.
But for now I think this makes sense – why bother compiling in guaranteed-to-be-dead code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think "make it just work on aarch64" is the approach. Ideally, we should also add a check that aarch64 builds are not allowed on the mainnet (see #5890), but I wouldn't worry too much about it.
I think we should land this roughly in this shape. One significant question/suggestions I have is that maybe it makes more sense to gate wasmer
on x64
, rather that on not aarch64
?
compile_error!("Wasmer does not support aarch64, but a force_wasmer* feature was passed to near-vm-runner"); | ||
|
||
VMKind::Wasmtime | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally prefer to have only one pub fn for_protocol_version(_protocol_version: ProtocolVersion) -> VMKind {
and pull conditional compilation inside function's body. This is much easier for an IDE to work with.
@@ -19,6 +19,7 @@ pub enum VMKind { | |||
} | |||
|
|||
impl VMKind { | |||
#[cfg(not(target_arch = "aarch64"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether the logic should be if x86_64
rather than if !aarch64
?
I'm excited to see progress here! This gives me some warnings when compiling. If we want full aarch64 support, we should also have CI checks done for that platform. Something to keep in mind for the future I guess. I don't know how to make suggestions for code lines you didn't touch. So I just put the diff here in the comment. Probably there is also a nicer way than my changes, but this at least shows how I got rid of all the warnings.
|
@@ -1,4 +1,6 @@ | |||
//! Tests that `CompiledContractCache` is working correctly. | |||
//! Tests that `CompiledContractCache` is working correctly. Currently testing only wasmer code, so disabled on aarch64 | |||
#![cfg(not(target_arch = "aarch64"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[cfg_attr(..., ignore)]
the tests instead.
Wasmer singlepass does not support aarch64, so disable it there and use wasmtime instead
Condition all wasmer compilation on not being on aarch64. This could also be done by using features, but then the reverse-dependencies would have to know about this limitation. Currently the code is a bit of a mess due to the amount of #![cfg()] elements, but hopefully this can be improved on later on by having one module with its own submodules per backend.
3275b32
to
4dffb65
Compare
I've just handled all the review comments, hopefully this is ready to land now :) |
4dffb65
to
7c49024
Compare
7c49024
to
0525ab9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (but obviously we need make sure this at least compiles)
Is there some trick we can use on Linux to check compilation for macs? Would something like cargo check -p near-vm-runner --target aarch64-apple-darwin
or cargo check -p aarch64-unknown-linux-gnu
added to CI help?
I think, long term, we should add mac-based CI, but that seems like a chore, so I am wondering if there's some "start small" we can do here? Better to do that as a follow up probably.
@@ -1,3 +1,6 @@ | |||
// Currently only testing wasmer code, so disabled outside of x86_64 | |||
#![cfg_attr(not(target_arch = "x86_64"), ignore)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, ignore
isn't supported for modules, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this doesn't seem to work at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right. @nagisa I'm cancelling the changes relating to #6200 (comment) ; as the file doesn't compile without wasmer at all (because the things it tests don't compile without wasmer)
That would indeed be nice. But we would still need an aarch64 machine, or otherwise make sure cross-compilation works. At least on my desktop, I'm missing dependencies for successful cross-compilation of wasmtime-runtime from x86 to aarch64. |
0525ab9
to
8d43764
Compare
This time to be as sure as I could that this builds properly, I ran something akin to s/x86_64/aarch64/ and checked things build correctly, so hopefully (once @jakmeier confirms on actual hardware) it should be good to go this time :) As for the CI, I'd think the best would be to get an aarch64 machine to run CI on, but I'm not sure how easy that'd be? (in particular, buildkite seems to be using distro=amazonlinux so I guess CI runs on aws in which case it should not be too hard as aws has aarch64 VMs, but I may be misunderstanding the setup… if that's too hard we can always use qemu to run the whole thing I guess) |
Yeah must be frustrating to code blindly... But I can confirm it now compiles and runs tests successfully. :) Unfortunately, there is a new aarch64 specific error in the nearcore build which has nothing to do with your change.
This also fails before your changes and was introduced with a change from only a few days ago. It means I cannot test if there are any errors hidden behind this failure. But at this point, I don't think that should stop this from merging. You have my approval (as a non code owener). |
Hmm can you try |
Yes that runs through without any errors or warnings. That's what I meant with "compiles and runs tests successfully". I also just created #6222 to track the other issue. |
Awesome thanks! |
https://wasmer.io/posts/wasmer-2.2 |
feat: aarch64 (m1) support
Wasmer singlepass does not support aarch64, so disable it there and use
wasmtime instead
feat: make everything compile on m1 (aarch64)
Condition all wasmer compilation on not being on aarch64. This could
also be done by using features, but then the reverse-dependencies would
have to know about this limitation.
Currently the code is a bit of a mess due to the amount of #![cfg()]
elements, but hopefully this can be improved on later on by having one
module with its own submodules per backend.
The support is still very experimental, but at least near-vm-runner's test will work now.
Related to #3803
The first commit is pretty much required, the second commit is this way in order for reverse-dependencies of near-vm-runner to not have to care about which features get enabled. An alternative could have been to make wasmer build on aarch64 (even if panicking on the first call), and the other alternative would have been to have all reverse-dependencies expose the wasm* features and ask that people on aarch64 manually toggle the proper features for things to work.