machinst: make it possible to test the new x64 backend#1880
machinst: make it possible to test the new x64 backend#1880bnjbvr merged 1 commit intobytecodealliance:masterfrom
Conversation
Subscribe to Label Actioncc @bnjbvr, @peterhuene DetailsThis issue or pull request has been labeled: "cranelift", "wasmtime:api"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
Not sure about removing |
| test run | ||
| set enable_simd | ||
| target x86_64 | ||
| target x86_64 haswell |
There was a problem hiding this comment.
My understanding of this is that adding haswell here means that only hosts compatible with the haswell flags will run this test (that's what we documented in https://github.com/bytecodealliance/wasmtime/blob/master/cranelift/docs/testing.md#test-run). Is that the intent? Or do we want to try to run on nehalem and above since that has the SSE flags needed? (It's probably hard to find hardware to trigger this error...).
There was a problem hiding this comment.
This is actually reflecting a reality that's implicit at the moment: the host machine must match some SSE level to be able to effectively run the tests. I think this was implicit because the native host ISA would parse the CPUID, and all the machines running this would have the SSE SSE prerequisites. But this means that somebody who'd try to run the test on a very old machine might not be able to, and run into illegal instructions exceptions there.
This was uncovered by the change in this patch that uses the target ISA to compile, as defined in the test file, rather than the host's one built and configured from within the Rust code. (To answer your question in another comment, this was required to make it possible to pass the use_new_backend flag to the test.)
I'll identify what the precise SSE flags are, and will try to mitigate the potential SSE flags mismatch issue.
There was a problem hiding this comment.
It's actually quite complicated, because the target specific flags are not to be queried outside the TargetIsa impl. I tried a lot of different things and lost a lot of time, so I went back to something simpler (this is testing only, after all): ensure the host is Nehalem at least, and by policy ensure that no one will add run tests that require more than that. We'll need to find a more long-term solution that does the right thing, probably with a full refactoring of the flags system.
4e3a966 to
e2e59b6
Compare
|
I'll drop the second commit for now; the parser and filetests codes are a bit entangled, and a combination of my wasting too much time on this + running in wasmtime should be sufficient make me think that this is lower value right now. |
This introduces two changes:
- first, a Cargo feature is added to make it possible to use the
Cranelift x64 backend directly from wasmtime's CLI.
- second, when passing a `cranelift-flags` parameter, and the given
parameter's name doesn't exist at the target-independent flag level, try
to set it as a target-dependent setting.
These two changes make it possible to try out the new x64 backend with:
cargo run --features experimental_x64 -- run --cranelift-flags use_new_backend=true -- /path/to/a.wasm
Right now, this will fail because most opcodes required by the
trampolines are actually not implemented yet.
test runmode. It uncovered one issue I'm not quite sure how to handle, but that's preexisting and it's not been a problem so far. @abrown, can you have a look, please?This should make it much easier to hack on and try the new work-in-progress x64 backend. Eventually we can delete the Cargo feature for wasmtime once the transition to the new backend is over.