Conversation
fitzgen
left a comment
There was a problem hiding this comment.
Thanks! This looks really good! Just a few nitpicks below before we merge this.
I'm excited to get a view of the CLIF alongside the Wasm and the asm!
crates/explorer/src/index.css
Outdated
| #wat { | ||
| width: 50%; | ||
| width: 33%; | ||
| height: 100%; | ||
| overflow: scroll; | ||
| } | ||
|
|
||
| #clif { | ||
| width: 33%; | ||
| height: 100%; | ||
| overflow: scroll; | ||
| } | ||
|
|
||
| #asm { | ||
| width: 50%; | ||
| width: 33%; |
There was a problem hiding this comment.
I don't think we want to apply this CSS all the time, only if we have CLIF to view. If we are doing wasmtime explore but using Winch instead of Cranelift, there won't be any CLIF to display, so it doesn't make sense to reserve a third of the screen for it.
I think we can resolve this by using flexbox instead of width percents. That way, if there are three things inside the container, they will each be a third of the width, and if there are two, they will each be half the width.
|
Thank you for taking the time to review my changes, I hope the code is fine now 🙂 |
fitzgen
left a comment
There was a problem hiding this comment.
This is looking really good! I tried running it locally, and when using Cranelift, this is super awesome. However, when using Winch, the explorer no longer works:
$ cargo run -- explore ~/scratch/hello.wasm -C compiler=winch
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.20s
Running `target/debug/wasmtime explore /home/nick/scratch/hello.wasm -C compiler=winch`
Error: clif output not supported
Could you make sure this doesn't break Winch support in wasmtime explore before we land this? Thanks!!
|
The dependency is now optional. I'm not sure how to deal with winch. Should I display 2 panes when compiling with winch or add clif emission support to winch ? |
|
@vulc41n the former: Winch doesn't have an intermediate IR like CLIF, it just goes directly from Wasm to assembly by design. So when using Winch, and we don't have CLIF available, we want to show the current view, and skip the CLIF pane in the explore UI. This is why we wanted to move to flex css, so that the panes would automatically fill the whole container width regardless whether we have CLIF or not. Does that all make sense? |
|
Thanks for the clarification, I've made the change 🙂 In the future, I hope I can help with more advanced stuff but there are a lot of concepts to grasp |
fitzgen
left a comment
There was a problem hiding this comment.
Fantastic, thanks! Appreciate your patience getting all the details sorted out here.
Is there a specific area you are interested in getting more involved with? I might be able to find something that could be a good on-ramp for you. |
Thanks, I appreciate. I was working on a source-to-source compiler, but right now I am unemployed and I just want to complete my experience with lower-level subjects. Anything about cranelift or winch would interest me 😀 |
|
@vulc41n, helping add aarch64 support to Winch could be a good fit for you: #8321 Each instruction is a relatively small amount of work but gets you more familiar with Wasm, aarch64, and Winch. Winch is simpler and easier to get up and running on than Cranelift. And there are a lot of instructions available for filling out! |
|
Ok, I will do this 😀 |
|
Feel free to ping me on github or zulip if you have any questions! |
As discussed in #8626, I added a new pane to the explorer to see the CLIF emitted.
It creates a temporary directory next to the html file, and discards it when it's done.
Feel free to criticize as it is my first contribution on this project 🙂