Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasmparser: feature gate Wasm simd support #1903

Merged
merged 84 commits into from
Nov 26, 2024

Conversation

Robbepop
Copy link
Collaborator

@Robbepop Robbepop commented Nov 12, 2024

Closes #1899.

This allows to feature gate parsing and validation of all 268 simd and relaxed-simd Wasm operators.

Goals: improved compile times and reduced artifact size.

Summary

  • Removes all simd and relaxed-simd definitions from macro_rules! for_each_operator.
  • Adds macro_rules! for_each_simd_operator.
  • Adds SimdOperator enum.
  • Adds VisitSimdOperator trait.
  • Adds VisitOperator::simd_visitor trait method with default implementation.
  • Adds simd crate feature, gating simd and relaxed-simd Wasm proposals support.
  • Adds Operator::Simd(SimdOperator) variant.
  • Puts #[non_exhaustive] on Operator enum.

Notes

  • There might be a performance regression since visiting SIMD operators in BinaryReader now is done via dynamic dispatch. Although it is likely that LLVM can optimize this. Benchmarks needed.
  • Usage of wasmparser as library is a bit more complex since we now have 2 generator macros instead of just one which is a bit ugly in my honest opinion.

Benchmarks

Command: cargo build -p wasmparser --no-default-features --features validate,features
Machine: Macbook M2 Pro

Compile Time !simd simd
debug 2.64s 2.90s
release 3.27s 3.64s
Artifact Size
debug 19MB 21MB
release 8.4MB 9.6MB

ToDo's

  • Fix other crates in the wasm-tools workspace.
    • wasm-encoder
    • wasmprinter
    • wasm-mutate
  • Properly feature gate using the new simd crate feature where possible.
  • Move some simd definitions into a single file to speed-up conditional compilation gating.
    • Move operator validator SIMD related functionality into a separate file.
      • This also helps with reviewability of the changes.
    • Move for_each_simd_operator macro into its own separate file.
    • Move BinaryReader::visit_0xfd_operator into its own separate file.
  • Write docs for new VisitSimdOperator trait and SimdOperator enum.
    • macro_rules! for_each_simd_operator
    • enum SimdOperator enum
    • trait VisitSimdOperator
    • VisitOperator::simd_visitor
  • Remove some duplicate code around macro expansions if possible.
    • Unfortunately this is not entirely preventable. Maybe @alexcrichton has an idea here and there.

@alexcrichton
Copy link
Member

Thanks again for pushing on this! Overall this is roughly what I was thinking, and while I'm ok with the current state of it I think it's worth reviewing the final end-state as well when all the CI is passing and everything is hooked up (e.g. #[cfg]s are in place and compile-time-wins are remeasured and such)

OperatorFactory now panics when trying to visit a SIMD operator while simd_visitor returns None.

Can this be fixed by marking enum Operator as #[non_exhaustive] and then conditionally including the SimdOperator enum?

There might be a performance regression since visiting SIMD operators in BinaryReader now is done via dynamic dispatch. Although it is likely that LLVM can optimize this. Benchmarks needed.

I'm not necessarily overly worried about this since simd instructions are likely a very small fraction of a larger module. Might be worth still trying to confirm one way or another, but I don't think it'd be the end of the world either way. When this might perhaps be an issue is with the GC proposal where if a similar strategy is taken for GC then that could have a significant impact since modules are probably all-GC or mostly-all-MVP for example.

Usage of wasmparser as library is a bit more complex since we now have 2 generator macros instead of just one which is a bit ugly in my honest opinion. Maybe it is possible to shrink to a single macro again but I don't know how at the moment.

I think it's pretty reasonable as is personally. There's two macros to invoke but they're both passed the same macro since they have the same syntax so while it's a bit more boilerplate that's sort of just par for the course with these sorts of refactorings and is a cost to acknowledge being placed on users where the benefit is being able to conditionally disable simd/etc.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Nov 18, 2024

@alexcrichton Thank you for the preliminary review of the WIP PR. :)

Thanks again for pushing on this! Overall this is roughly what I was thinking, and while I'm ok with the current state of it I think it's worth reviewing the final end-state as well when all the CI is passing and everything is hooked up (e.g. #[cfg]s are in place and compile-time-wins are remeasured and such)

I will be working on that now. I just wanted to know if the current structure is as you imagined which seems to be the case.

Can this be fixed by marking enum Operator as #[non_exhaustive] and then conditionally including the SimdOperator enum?

Should be possible indeed.

When this might perhaps be an issue is with the GC proposal where if a similar strategy is taken for GC then that could have a significant impact since modules are probably all-GC or mostly-all-MVP for example.

We only do all this acrobatics because there are so many simd operators. From what I know, no other Wasm proposal has nearly as many operators as simd, not even the gc proposal. So the first question is if we actually need to conditionally compile gc support. So far I do not think so.

I think it's pretty reasonable as is personally. There's two macros to invoke but they're both passed the same macro since they have the same syntax so while it's a bit more boilerplate that's sort of just par for the course with these sorts of refactorings and is a cost to acknowledge being placed on users where the benefit is being able to conditionally disable simd/etc.

Okay thank you for your view on this. I agree!

@alexcrichton
Copy link
Member

I did a small analysis of opcodes in wasmparser recently and this is what I got:

wasm opcodes

basically while simd is big it's only ~1/3. Roughly 1/4 of all opcodes are mvp and threads+shared-everything-threads are becoming a relatively large slice of the pie

@Robbepop
Copy link
Collaborator Author

Robbepop commented Nov 18, 2024

I did a small analysis of opcodes in wasmparser recently and this is what I got:

wasm opcodes

basically while simd is big it's only ~1/3. Roughly 1/4 of all opcodes are mvp and threads+shared-everything-threads are becoming a relatively large slice of the pie

Oh, that is very interesting. So is it true that threads+shared-everything+gc is (going to be) used commonly together just like simd+relaxed-simd so should be grouped?

@alexcrichton
Copy link
Member

Oh not necessarily, and shared-everything-threads is still under development itself. While simd is a relatively obvious choice of "can split this off" I'm not sure if there are other reasonable things to split off in chunks at this time.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Nov 25, 2024

What I might envision as the next transition step might be:

* `for_each_visit_operator!` - used with `trait VisitOperator` (no simd)

* `for_each_simd_visit_operator!` - used with `trait VisitSimdOperator` (only simd)

* `for_each_operator!` - repurpose this to be used with `Operator` (both simd + others)

Associating the new macros with their respective intended use is a brilliant idea. I was already struggeling how to best expose and name those new macros but this is a solution I am very willing to put forward.

A possible future change would be to update the syntax of each macro possible where for_each_operator may not want the visit_* methods as arguments and the others may not want the camel case struct names as arguments. That's fine to defer to later though.

Yes, this indeed might be a good idea. I agree that a follow-up PR is a better place for those improvements since this PR is already so big.

Robbepop and others added 24 commits November 26, 2024 15:03
The wasmparser crate now exposes 3 different for_each macros:

- for_each_operator: Same as before the PR. Iterates over _all_ operators. This is going to be used to implement a routine for all Operator enum variants.
- for_each_visit_oeprator: This is going to be used to implement the `VisitOperator` trait.
- for_each_visit_simd_operator: This is going to be used to implement the `VisitSimdOperator` trait.
This mostly reverts the changes done in past commits of the PR.
* Revert `crates/wasm-encoder/src/reencode.rs` back to `main` version
  (no changes needed any more)
* Minor whitespace/style changes
* Document the macros some more in `wasmparser/src/lib.rs`
* Reorganize macro-defining-macros to have more predictable names.
@alexcrichton alexcrichton added this pull request to the merge queue Nov 26, 2024
Merged via the queue into bytecodealliance:main with commit c0eab4d Nov 26, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wasmparser: Should we put 128-bit SIMD behind a crate feature?
2 participants