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.

Sign up for GitHub

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

Jump to bottom

wasmparser: feature gate Wasm simd support #1903

Open
wants to merge 57 commits into
base: main
Choose a base branch
from
Open

wasmparser: feature gate Wasm simd support #1903

wants to merge 57 commits into from

Conversation

Robbepop
Copy link
Contributor

@Robbepop Robbepop commented Nov 12, 2024 •
edited
Loading

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
Contributor Author

Robbepop commented Nov 18, 2024 •
edited
Loading

@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
Contributor Author

Robbepop commented Nov 18, 2024 •
edited
Loading

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 Robbepop marked this pull request as ready for review November 20, 2024 00:33
@Robbepop
Copy link
Contributor Author

@alexcrichton this PR is ready for a proper review now.

@Robbepop Robbepop changed the title wasmparser: feature gate Wasm simd support (WIP) wasmparser: feature gate Wasm simd support Nov 20, 2024
alexcrichton
alexcrichton reviewed Nov 21, 2024
View reviewed changes
crates/wasm-encoder/Cargo.toml Outdated Show resolved Hide resolved
crates/wasm-mutate/src/mutators/modify_const_exprs.rs
@@ -76,7 +76,7 @@ impl<'cfg, 'wasm> Reencode for InitTranslator<'cfg, 'wasm> {
O::RefNull { .. } | O::I32Const { value: 0 | 1 } | O::I64Const { value: 0 | 1 } => true,
O::F32Const { value } => value.bits() == 0,
O::F64Const { value } => value.bits() == 0,
O::V128Const { value } => value.i128() == 0,
O::Simd(SimdOperator::V128Const { value }) => value.i128() == 0,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, what do you think of throwing all the simd ops back into Operator? It's already #[non_exhaustive] and would be nice to have a bit less churn as well in theory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not yet sure if this works as intended due to the split into 2 macros and the problem that you cannot have macro invokation at enum variant definition position but I ll see how it goes. A downside to this approach is that this would result in tons of cfgs.

Cargo.toml Outdated Show resolved Hide resolved
crates/wasmprinter/Cargo.toml Outdated Show resolved Hide resolved
@Robbepop
unconditionally enable simd for wasmprinter
cec6bb7
@Robbepop
unconditionally enable simd for wasm-encoder
1dfbf6e
@Robbepop
remove wasm-tools simd feature (enable by default)
a98227f
@Robbepop
use macros generate for_each_operator macros
efa3d43 
- This does not yet provide a for_each_operator macro in for !simd mode but that should be easily added.
- This is WIP in that is currently does not handle the documentation of the macros well. We might need to also generate the macro docs in the macros themselves.
@Robbepop
remove comment out line
887a8cf
@Robbepop
put docs on the exported macros
cbbc575
@Robbepop
fix macro imports
ba48386
@Robbepop
fix bugs in generator macros
6b9a883
@Robbepop
improve compile times for macros
91ddc75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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