[WASM] fix unconditional SIMD inclusion in WASM binary #16
Conversation
|
cc/ @taj-p |
|
|
||
| #[inline] | ||
| fn vectorize<F: FnOnce() -> R, R>(self, f: F) -> R { | ||
| #[target_feature(enable = "simd128")] |
There was a problem hiding this comment.
Don't we need the target_arch here?
There was a problem hiding this comment.
I'm going to have to think about this a bit more in the WASM case.
Although I agree that the vectorize method exists in order to force the usage of the simd128 target feature, hence allowing vectorization, it's not actually possible in the WASM case to even call this without the target feature already being set - since WASM chooses whether SIMD is allowed at compile time.
I.e. this vectorize method ends up within generated/wasm.rs, which is gated behind a #[cfg(all(target_arch = "wasm32", target_feature = "simd128"))].
I think this API doesn't make sense for WASM, as in WASM you'd reach for target_feature if you wanted vectorization. I'm leaning towards doing nothing at the moment. Maybe I could add a comment?
There was a problem hiding this comment.
Sure, just was wondering!
There was a problem hiding this comment.
It's a really good questions :D
Thank you for reviewing!
) ### Context This is a bit of a subtle fix that follows https://github.com/raphlinus/fearless_simd/pull/16 The important excerpts from the WebAssembly spec. > "[Modules](https://www.w3.org/TR/wasm-core-2/index.html#syntax-module) are valid when all the components they contain are valid." > ~[link](https://www.w3.org/TR/wasm-core-2/#modules%E2%91%A2) Thus, for a browser (or WebAssembly runtime) that doesn't support SIMD, the module validation check would fail because: - `v128` type would be invalid and wouldn't exist in that WebAssembly context. - `expr` must also be valid, so instructions over f32x4 would fail validation. ### Why Prior to the fearless_simd fix, it was possible via `target_feature` to accidentally leak SIMD instructions into the binary. Even if these instructions are never executed, their mere presence would result in the WASM binary failing to validate. ### Changes This PR was implemented in a test-driven style. First I implemented the test: - The test only runs if the `simd128` target feature is not requested in the rustc flags. - The test self-introspects the test wasm binary and validates the binary against a validator that doesn't contain SIMD support. This test failed (because of the leaking SIMD instructions). Hence, on a browser that didn't support SIMD we would crash. (Not good for consumers of vello using WASM asking for a scalar binary). Example test failure: <img width="1022" alt="Screenshot 2025-06-27 at 3 21 59 pm" src="https://github.com/user-attachments/assets/fe886294-5cb1-40ea-b97d-0f7b10ce62b5" /> Then I bumped up fearless_simd, and the test passes! ### Test plan This PR is effectively a test to prevent regressions.
Context
I made a mistake using
#[target_feature(enable = "simd128")]because it means the enclosed code is compiled using the simd128 feature flag. This can result in binaries containing SIMD. Even if the SIMD is never executed, older browsers may fail to validate the WASM binary if it includes SIMD instructions.Thus, this PR removes all usages of target_feature for WASM, instead opting into conditional compilation.
Test plan
This really needs an automated test, but I haven't found a good way to test this yet.
I manually tested by running the following command:
Now returns
NO SIMD, when previously it returnedSIMD FOUND.Returns
SIMD FOUNDbecause the+simd128feature was requested.