adambratschikaye edited PR #7766.
adambratschikaye has marked PR #7766 as ready for review.
adambratschikaye requested elliottt for a review on PR #7766.
adambratschikaye requested wasmtime-compiler-reviewers for a review on PR #7766.
adambratschikaye requested alexcrichton for a review on PR #7766.
adambratschikaye requested wasmtime-core-reviewers for a review on PR #7766.
adambratschikaye commented on PR #7766:
With these changes I was able to get this example of performing wasmtime compilation within wasm to work. However there was one problem with is that the compilation of wasmtime to wasm32 contains a function with ~60,000 locals and
wasmparser
has a hard limit of 50,000 locals per function.So I'd like to add a test that does the compilation within wasm, but I still need to figure out how to handle that limit on locals.
adambratschikaye updated PR #7766.
alexcrichton commented on PR #7766:
That's awesome! I think you're perhaps the first person ever to execute a cross-compilation using Cranelift from a 32-bit architecture to a 64-bit architecture!
I'll dig into this today and give a review.
alexcrichton submitted PR review:
Thanks again for this! Reading over it, everything looks at a high level ok here, but if you're ok with it I'd like to brainstorm a bit on the internal organization here. My goal here would be to cut down on "complexity" (a loose term here...) which kind of translates to minimizing
#[cfg]
and other things. Some possible ideas:
Could the
runtime
module, at its root, have everything reexported of it's internals? My hope would be that inlib.rs
we'd have:
rust #[cfg(feature = "runtime")] mod runtime; #[cfg(feature = "runtime")] pub use runtime::*;
I think
cargo fmt
has issues with modules defined via macro expansions so moving themod runtime
out of a macro may benefit that niche use case.I think it's ok to move some more things into
runtime
that are technically agnostic likelimits
,resources
, andstack
.- Can you add a builder to CI to build the wasmtime crate for the wasm32-wasi target with the
runtime
feature disabled?Splitting
engine.rs
feels not great since it's now so spread out (and requires a large struct with a bunch ofpub(crate)
which never feels great. This might be something where it may be worth some more#[cfg]
. Do you know if a design such as this would work?
rust impl Engine { /* common methods */ } #[cfg(feature = "runtime")] impl Engine { /* runtime methods */ } #[cfg(feature = "compile")] impl Engine { /* compilation methods */ }
that might help keep everything in a single file, albeit there'd probably be a lot of
#[cfg]
related touse
directives at the top of the file perhaps.Do you think it would make sense to have a top level
mod compile;
with all compilation-related bits under that? Right now it's sort of spread out at the root it seems.
alexcrichton submitted PR review:
Thanks again for this! Reading over it, everything looks at a high level ok here, but if you're ok with it I'd like to brainstorm a bit on the internal organization here. My goal here would be to cut down on "complexity" (a loose term here...) which kind of translates to minimizing
#[cfg]
and other things. Some possible ideas:
Could the
runtime
module, at its root, have everything reexported of it's internals? My hope would be that inlib.rs
we'd have:
rust #[cfg(feature = "runtime")] mod runtime; #[cfg(feature = "runtime")] pub use runtime::*;
I think
cargo fmt
has issues with modules defined via macro expansions so moving themod runtime
out of a macro may benefit that niche use case.I think it's ok to move some more things into
runtime
that are technically agnostic likelimits
,resources
, andstack
.- Can you add a builder to CI to build the wasmtime crate for the wasm32-wasi target with the
runtime
feature disabled?Splitting
engine.rs
feels not great since it's now so spread out (and requires a large struct with a bunch ofpub(crate)
which never feels great. This might be something where it may be worth some more#[cfg]
. Do you know if a design such as this would work?
rust impl Engine { /* common methods */ } #[cfg(feature = "runtime")] impl Engine { /* runtime methods */ } #[cfg(feature = "compile")] impl Engine { /* compilation methods */ }
that might help keep everything in a single file, albeit there'd probably be a lot of
#[cfg]
related touse
directives at the top of the file perhaps.Do you think it would make sense to have a top level
mod compile;
with all compilation-related bits under that? Right now it's sort of spread out at the root it seems.
alexcrichton created PR review comment:
Could you add a doc-block here and above
runtime
below indicating what the feature is?
alexcrichton created PR review comment:
Mind throwing a comment above this indicating why it's here?
alexcrichton commented on PR #7766:
So I'd like to add a test that does the compilation within wasm, but I still need to figure out how to handle that limit on locals.
Can you paste an example of buliding wasmtime like this? I checked out your repo and pointed it at this branch and this passed:
$ wasm-tools validate ./target/wasm32-wasi/release/wasmtime-in-wasm.wasm
but I expected that it might otherwise fail
adambratschikaye commented on PR #7766:
Thanks, all those comments seem reasonable and I'll look into them.
adambratschikaye updated PR #7766.
adambratschikaye updated PR #7766.
adambratschikaye updated PR #7766.
adambratschikaye requested wasmtime-default-reviewers for a review on PR #7766.
adambratschikaye updated PR #7766.
adambratschikaye updated PR #7766.
adambratschikaye updated PR #7766.
adambratschikaye updated PR #7766.
adambratschikaye updated PR #7766.
adambratschikaye updated PR #7766.
adambratschikaye updated PR #7766.
adambratschikaye submitted PR review.
adambratschikaye created PR review comment:
Yep, added one.
adambratschikaye submitted PR review.
adambratschikaye created PR review comment:
I added one for
runtime
. I think this feature isn't actually necessary - I modified the cranelift build so that ifall-arch
is enabled it doesn't complain if it can't build for the host arch.
adambratschikaye commented on PR #7766:
Ok, I've made most of the suggested changes:
- Could the
runtime
module, at its root, have everything reexported of it's internals? My hope would be that inlib.rs
we'd have:
rust #[cfg(feature = "runtime")] mod runtime; #[cfg(feature = "runtime")] pub use runtime::*;
I thinkcargo fmt
has issues with modules defined via macro expansions so moving themod runtime
out of a macro may benefit that niche use case.Yes this seems to be cleaner.
- I think it's ok to move some more things into
runtime
that are technically agnostic likelimits
,resources
, andstack
.I moved these three as well as
coredump
anddebug
.
- Can you add a builder to CI to build the wasmtime crate for the wasm32-wasi target with the
runtime
feature disabled?I added an extra build job in the CI (that runs on x86 linux). Hope this makes sense within the general CI setup of the project.
- Splitting
engine.rs
feels not great since it's now so spread out (and requires a large struct with a bunch ofpub(crate)
which never feels great. This might be something where it may be worth some more#[cfg]
. Do you know if a design such as this would work?
rust impl Engine { /* common methods */ } #[cfg(feature = "runtime")] impl Engine { /* runtime methods */ } #[cfg(feature = "compile")] impl Engine { /* compilation methods */ }
that might help keep everything in a single file, albeit there'd probably be a lot of#[cfg]
related touse
directives at the top of the file perhaps.Yep this works fine, except I didn't add a
compile
feature. It would just be the same asany(feature = "cranelift", feature = "winch")
so I just used that everywhere.
- Do you think it would make sense to have a top level
mod compile;
with all compilation-related bits under that? Right now it's sort of spread out at the root it seems.I put some things that were at the top-level under the
compile
module. But there's still a bunch of compilation specific stuff undercfg
directives in the runtime module. Many of these seem like they wouldn't make sense to move over to thecompile
because they rely on types that wouldn't be used without theruntime
feature. For example,Func::new
requires compilation, but I don't see how it could be used without theruntime
feature (there are similar examples forLinker
,Module
, andComponent
).
adambratschikaye commented on PR #7766:
Can you paste an example of buliding wasmtime like this? I checked out your repo and pointed it at this branch and this passed:
$ wasm-tools validate ./target/wasm32-wasi/release/wasmtime-in-wasm.wasm
but I expected that it might otherwise fail
Ah yep, that's working for me as well. Doing the debug build hits the limit though:
❯ cargo build Finished dev [unoptimized + debuginfo] target(s) in 0.16s ❯ wasm-tools validate target/wasm32-wasi/debug/wasmtime-in-wasm.wasm error: func 35030 failed to validate Caused by: 0: too many locals: locals exceed maximum (at offset 0x111420c)
Would it make sense to add that code as an example along with a CI job do the compilation in Wasm and then execute on the host system? And would you want that job to run on all hosts in the matrix?
alexcrichton submitted PR review:
Ok thanks again for you work on this, it's all looking great to me! I've got some minor nits and things but I think it's probably best to go ahead and land this and I can follow-up myself with anything else I'd like. I'd also like to get the
wasmtime
CLI itself compiling to wasm with just the cranelift/winch features enabled, but no need to do that here either! I think there's a rebase conflict but otherwise I'm happy to throw this at the merge queue.Would it make sense to add that code as an example along with a CI job do the compilation in Wasm and then execute on the host system? And would you want that job to run on all hosts in the matrix?
I think so yeah, but I'll do that as part of a follow-up to get the
wasmtime
CLI itself compiling. I tried for a bit to figure out what function was causing issues unsuccessfully, but this is also ok to work on as a follow-up.My guess is that it's a function in cranelift-codegen for lowering since that's the main source of "big generated code". I have yet to confirm this though.
alexcrichton commented on PR #7766:
Ah ok so I found the function:
$ wasm-tools validate ./target/wasm32-wasi/debug/wasmtime-in-wasm.wasm error: func 39100 failed to validate Caused by: 0: too many locals: locals exceed maximum (at offset 0x12ebabe) $ wasm-tools print ./target/wasm32-wasi/debug/wasmtime-in-wasm.wasm --skeleton | rg 39100 (func $_ZN4core6option19Option$LT$$RF$T$GT$6cloned17h3ad039100bcc27f6E (;11910;) (type 2) (param i32 i32) ...) (func $_ZN17cranelift_codegen2ir7builder11InstBuilder8f64const17h4e1391004dd942baE (;18861;) (type 4) (param i32 i32) (result i32) ...) (func $_ZN17cranelift_codegen4opts14generated_code20constructor_simplify17h2e155f4b5f5d37f9E (;39100;) (type 9) (param i32 i32 i32) ...) $ rustfilt _ZN17cranelift_codegen4opts14generated_code20constructor_simplify17h2e155f4b5f5d37f9E cranelift_codegen::opts::generated_code::constructor_simplify
That corresponds to the ISLE-generated code for optimizing CLIF. I'm not personally aware of any low-hanging fruit on optimizing the output of ISLE for debug builds, so this may be a case where
cranelift-codegen
the crate needs to be optimized, even in debug builds.
adambratschikaye updated PR #7766.
adambratschikaye commented on PR #7766:
Ok thanks again for you work on this, it's all looking great to me! I've got some minor nits and things but I think it's probably best to go ahead and land this and I can follow-up myself with anything else I'd like. I'd also like to get the
wasmtime
CLI itself compiling to wasm with just the cranelift/winch features enabled, but no need to do that here either! I think there's a rebase conflict but otherwise I'm happy to throw this at the merge queue.I resolved the conflicts. Happy to follow up with compiling the CLI to wasm as well.
alexcrichton submitted PR review.
alexcrichton commented on PR #7766:
Ok sounds good! I don't necessarily have a concrete use case in mind but it seems like a neat way to demo how Cranelift can be compiled to wasm. I'll poke a bit more at the internals of the
wasmtime
crate and leave the CLI bits to you.
alexcrichton merged PR #7766.
alexcrichton commented on PR #7766:
I was curious to play around with this and went a bit far down the rabbit hole! I was able to hack things up and get things working though with https://github.com/bytecodealliance/wasmtime/pull/7842 and https://github.com/bytecodealliance/wasmtime/pull/7844, however. That's perhaps a bit of a taste of how cross-compilation, especially across pointer widths, is not the most well trodden path in Wasmtime so there may be a few other lingering bugs. Mostly just something to be aware of, and I'd hope that we can start adding regression tests in CI once we've got CLI support!
Last updated: Dec 23 2024 at 12:05 UTC