Stream: git-wasmtime

Topic: wasmtime / issue #3809 Minimum x86_64 feature support req...

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 15:15):

alexcrichton opened issue #3809:

With https://github.com/bytecodealliance/wasmtime/pull/3800 on oss-fuzz last night it quickly found a few "bugs" in the cranelift backend for x86_64, primarily around SIMD support. This led me to raise a question, however, as to whether or not we want to consider these issues bugs. The crashes all have the same pattern where we're forcibly disabling processor features and testing whether wasm still works.

Currently in Wasmtime it appears that at least for some instructions the SSE 4.1 extensions are required. If those are not present then two crashes found so far are:

With this in mind I'm wondering if it makes sense to consider these bugs or not? There's nothing wrong with the SSE 4.1 codegen this is only a problem for when that feature isn't available we're unable to generate code. Is this something we want to consider as a bug? Or alternatively we can also simply declare that running Wasmtime with the SIMD wasm feature enabled requires SSE 4.1 on x86_64 and we can provide a more first-class error in Wasmtime when a module is attempted to be used without SSE 4.1

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 15:15):

alexcrichton labeled issue #3809:

With https://github.com/bytecodealliance/wasmtime/pull/3800 on oss-fuzz last night it quickly found a few "bugs" in the cranelift backend for x86_64, primarily around SIMD support. This led me to raise a question, however, as to whether or not we want to consider these issues bugs. The crashes all have the same pattern where we're forcibly disabling processor features and testing whether wasm still works.

Currently in Wasmtime it appears that at least for some instructions the SSE 4.1 extensions are required. If those are not present then two crashes found so far are:

With this in mind I'm wondering if it makes sense to consider these bugs or not? There's nothing wrong with the SSE 4.1 codegen this is only a problem for when that feature isn't available we're unable to generate code. Is this something we want to consider as a bug? Or alternatively we can also simply declare that running Wasmtime with the SIMD wasm feature enabled requires SSE 4.1 on x86_64 and we can provide a more first-class error in Wasmtime when a module is attempted to be used without SSE 4.1

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 15:15):

alexcrichton labeled issue #3809:

With https://github.com/bytecodealliance/wasmtime/pull/3800 on oss-fuzz last night it quickly found a few "bugs" in the cranelift backend for x86_64, primarily around SIMD support. This led me to raise a question, however, as to whether or not we want to consider these issues bugs. The crashes all have the same pattern where we're forcibly disabling processor features and testing whether wasm still works.

Currently in Wasmtime it appears that at least for some instructions the SSE 4.1 extensions are required. If those are not present then two crashes found so far are:

With this in mind I'm wondering if it makes sense to consider these bugs or not? There's nothing wrong with the SSE 4.1 codegen this is only a problem for when that feature isn't available we're unable to generate code. Is this something we want to consider as a bug? Or alternatively we can also simply declare that running Wasmtime with the SIMD wasm feature enabled requires SSE 4.1 on x86_64 and we can provide a more first-class error in Wasmtime when a module is attempted to be used without SSE 4.1

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 15:37):

bjorn3 commented on issue #3809:

For cg_clif it would be nice if Cranelift could always legalize simd operations too big for the target arch to smaller simd operations or even scalar operations as necessary. The code has to exist somewhere as portable-simd allows vectors bigger than the natively supported size. Putting it in Cranelift allows others to reuse the same code and allows cg_clif to not contain a hardcoded list of which operation of which size is allowed when each specific target feature is enabled.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 16:48):

cfallin commented on issue #3809:

Aspirationally I think it would be nice to have the ability to run all features on any x86_64 machine; that means SSE2 (required by x86_64 ISA) only. But practically speaking, SSE4.1 was introduced with Core 2 in 2007-ish and so is 14-15 years old at this point; if we have to require it when SIMD is turned on, at least for now, that's not the end of the world. (Almost all extant x86_64 chips will be newer than that -- we'd just be excluding the very early Athlon 64s and late-model Pentium 4s.)

In any case, we should fail more nicely than an internal panic, as you say -- I can do some thinking about where the right place might be. Perhaps we can take an isa_flags with the verifier somehow; or perhaps we can define a "supports all CLIF vector instructions" helper function on the target that tells the producer whether the current settings are sufficient, and then check this against SIMD-enabled in the Wasmtime cranelift crate somewhere.

I'll file an issue to track making SIMD "SSE2-clean" as well.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 17:04):

alexcrichton commented on issue #3809:

If we decide to go the route of "simd in wasmtime simply requires these features" then I could see that either happening in Wasmtime or in Cranelift, I dont think it necessarily is required to be in Cranelift. I suspect it might actually be easiest in Wasmtime to avoid having to keep everything in sync about individual instructions and at the Cranelift layer we could point existing panics to #3810 and for Wasmtime you'd get a first-class error on creating an Engine for a target which enables the simd proposal but doesn't support SSE 4.1.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 17:18):

bjorn3 commented on issue #3809:

context.compile returns a CodegenResult. The Unsupported variant of CodegenError says that it is meant for things not supported by the backend or for which the required feature is not enabled. I think Cranelift should return Unsupported rather than panic if it is intentionally unsupported for the current set of enabled features.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 18:26):

abrown commented on issue #3809:

Chiming in late, but here's some context from the Wasm SIMD sub-group: at some point we reached a general consensus that SSE 4.1 would form the baseline for instructions added to the proposal, e.g, see this comment (probably documented more clearly somewhere else):

... for SSE4.1 which we are looking at as a baseline for this proposal on Intel architectures

I agree with @cfallin that #3810 is a "nice to have" but most hardware running has at least SSE 4.1. Whether we link Wasm SIMD and SSE 4.1 in Wasmtime (as @alexcrichton has suggested) or in Cranelift (as @bjorn3 has suggested) is not too important to me but if either of you implements this could you tag me in the PR so I can take a look?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 18:27):

abrown edited a comment on issue #3809:

Chiming in late, but here's some context from the Wasm SIMD sub-group: at some point we reached a general consensus that, on x86, SSE 4.1 would form the baseline for instructions added to the proposal, e.g, see this comment (probably documented more clearly somewhere else):

... for SSE4.1 which we are looking at as a baseline for this proposal on Intel architectures

I agree with @cfallin that #3810 is a "nice to have" but most hardware running has at least SSE 4.1. Whether we link Wasm SIMD and SSE 4.1 in Wasmtime (as @alexcrichton has suggested) or in Cranelift (as @bjorn3 has suggested) is not too important to me but if either of you implements this could you tag me in the PR so I can take a look?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 19:08):

alexcrichton commented on issue #3809:

Ok I went ahead and posted https://github.com/bytecodealliance/wasmtime/pull/3814 and figure this issue can be closed otherwise. If in practice no one's actually running without SSE 4.1 then I don't think it's a big issue to leave these panics.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 20:25):

bjorn3 commented on issue #3809:

I actually had someone complain about cg_clif not working on cpu's before nehalem due to enabling the nehalem feature for simd support: https://github.com/bjorn3/rustc_codegen_cranelift/issues/1148

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 20:53):

alexcrichton closed issue #3809:

With https://github.com/bytecodealliance/wasmtime/pull/3800 on oss-fuzz last night it quickly found a few "bugs" in the cranelift backend for x86_64, primarily around SIMD support. This led me to raise a question, however, as to whether or not we want to consider these issues bugs. The crashes all have the same pattern where we're forcibly disabling processor features and testing whether wasm still works.

Currently in Wasmtime it appears that at least for some instructions the SSE 4.1 extensions are required. If those are not present then two crashes found so far are:

With this in mind I'm wondering if it makes sense to consider these bugs or not? There's nothing wrong with the SSE 4.1 codegen this is only a problem for when that feature isn't available we're unable to generate code. Is this something we want to consider as a bug? Or alternatively we can also simply declare that running Wasmtime with the SIMD wasm feature enabled requires SSE 4.1 on x86_64 and we can provide a more first-class error in Wasmtime when a module is attempted to be used without SSE 4.1

Last updated: Nov 22 2024 at 17:03 UTC