wasmparser: merge VisitSimdOperator back into VisitOperator

The `simd` feature remains in place, but now does not affect the API in
this way. Rather parts of implementation pertaining to SIMD in
wasmparser get disabled, reducing the compilation time and artifact
size impact slightly, though less than in the original approach.

The results on my machine are roughly:

* origin/main simd: 4.26s producing 30M .rlib
* origin/main !simd: 3.49s producing 23M .rlib
* this simd: 4.19s producing a 26M .rlib
* this !simd: 3.96s producing a 25M .rlib

I'm not quite sure why the rlib size decreased so significantly when the
SIMD feature is enabled (I guess less inlining is occurring?) and there
is indeed a .5s hit on the compilation time to this PR.

Are those half a second worth a clunkier API? I'm not convinced :)

Author: nagisa

crates/wasmparser/src/lib.rs

@@ -827,21 +827,14 @@ macro_rules! define_for_each_non_simd_operator {
                 $m! { $($t)* }
             }
         }
-
-        // When simd is disabled then this macro is additionally the
-        // `for_each_operator!` macro implementation
-        #[cfg(not(feature = "simd"))]
-        #[doc(hidden)]
-        pub use _for_each_visit_operator_impl as _for_each_operator_impl;
     };
 }
 _for_each_operator_group!(define_for_each_non_simd_operator);
 
 /// When the simd feature is enabled then `_for_each_operator_impl` is defined
 /// to be the same as the above `define_for_each_non_simd_operator` macro except
 /// with all proposals thrown in.
-#[cfg(feature = "simd")]
-macro_rules! define_for_each_operator_impl_with_simd {
+macro_rules! define_for_each_operator_impl {
     (
         $(
             @$proposal:ident {
@@ -864,14 +857,12 @@ macro_rules! define_for_each_operator_impl_with_simd {
         }
     };
 }
-#[cfg(feature = "simd")]
-_for_each_operator_group!(define_for_each_operator_impl_with_simd);
+_for_each_operator_group!(define_for_each_operator_impl);
 
 /// Helper macro to define the `_for_each_simd_operator_impl` macro.
 ///
 /// This is basically the same as `define_for_each_non_simd_operator` above
 /// except that it's filtering on different proposals.
-#[cfg(feature = "simd")]
 macro_rules! define_for_each_simd_operator {
     // Switch to "tt muncher" mode
     (@ $($t:tt)*) => {define_for_each_simd_operator!(filter [] @ $($t)*);};
@@ -924,7 +915,7 @@ macro_rules! define_for_each_simd_operator {
         }
     };
 }
-#[cfg(feature = "simd")]
+
 _for_each_operator_group!(define_for_each_simd_operator);
 
 /// Used to implement routines for the [`Operator`] enum.
@@ -1272,7 +1263,6 @@ pub use _for_each_visit_operator_impl as for_each_visit_operator;
 ///     wasmparser::for_each_visit_simd_operator!(define_visit_simd_operator);
 /// }
 /// ```
-#[cfg(feature = "simd")]
 #[doc(inline)]
 pub use _for_each_visit_simd_operator_impl as for_each_visit_simd_operator;
 

crates/wasmparser/src/readers/core/operators.rs

@@ -10,8 +10,6 @@ use super::{
     operators::{ty_to_str, OperatorValidator, OperatorValidatorAllocations},
     types::{CoreTypeId, EntityType, RecGroupId, TypeAlloc, TypeList},
 };
-#[cfg(feature = "simd")]
-use crate::VisitSimdOperator;
 use crate::{
     limits::*, BinaryReaderError, ConstExpr, Data, DataKind, Element, ElementKind, ExternalKind,
     FuncType, Global, GlobalType, HeapType, MemoryType, RecGroup, RefType, Result, SubType, Table,
@@ -234,6 +232,15 @@ impl ModuleState {
                 self.ops.with_resources(&self.resources, self.offset)
             }
 
+            #[cold]
+            #[cfg(not(feature = "simd"))]
+            fn simd_unavailable(&mut self) -> Result<()> {
+                Err(BinaryReaderError::new(
+                    format!("SIMD not enabled at compile time"),
+                    self.offset,
+                ))
+            }
+
             fn validate_extended_const(&mut self, op: &str) -> Result<()> {
                 if self.ops.features.extended_const() {
                     Ok(())
@@ -326,12 +333,28 @@ impl ModuleState {
         #[cfg_attr(not(feature = "simd"), allow(unused_macro_rules))]
         macro_rules! define_visit_operator {
             ($(@$proposal:ident $op:ident $({ $($arg:ident: $argty:ty),* })? => $visit:ident ($($ann:tt)*))*) => {
-                $(
-                    #[allow(unused_variables)]
-                    fn $visit(&mut self $($(,$arg: $argty)*)?) -> Self::Output {
-                        define_visit_operator!(@visit self $visit $($($arg)*)?)
-                    }
-                )*
+                $(define_visit_operator!(expand @$proposal $op $({ $($arg: $argty),* })? => $visit ($($ann)*));)*
+            };
+
+            (expand @simd $op:ident $({ $($arg:ident: $argty:ty),* })? => $visit:ident ($($ann:tt)*)) => {
+                #[cfg(not(feature = "simd"))]
+                #[allow(unused_variables)]
+                fn $visit(&mut self $($(,$arg: $argty)*)?) -> Self::Output {
+                    self.simd_unavailable()
+                }
+
+                #[cfg(feature = "simd")]
+                #[allow(unused_variables)]
+                fn $visit(&mut self $($(,$arg: $argty)*)?) -> Self::Output {
+                    define_visit_operator!(@visit self $visit $($($arg)*)?)
+                }
+            };
+
+            (expand @$proposal:ident $op:ident $({ $($arg:ident: $argty:ty),* })? => $visit:ident ($($ann:tt)*)) => {
+                #[allow(unused_variables)]
+                fn $visit(&mut self $($(,$arg: $argty)*)?) -> Self::Output {
+                    define_visit_operator!(@visit self $visit $($($arg)*)?)
+                }
             };
 
             // These are always valid in const expressions
@@ -348,7 +371,7 @@ impl ModuleState {
                 $self.validator().visit_f64_const($val)
             }};
             (@visit $self:ident visit_v128_const $val:ident) => {{
-                $self.validator().simd_visitor().unwrap().visit_v128_const($val)
+                $self.validator().visit_v128_const($val)
             }};
             (@visit $self:ident visit_ref_null $val:ident) => {{
                 $self.validator().visit_ref_null($val)
@@ -444,18 +467,7 @@ impl ModuleState {
         impl<'a> VisitOperator<'a> for VisitConstOperator<'a> {
             type Output = Result<()>;
 
-            #[cfg(feature = "simd")]
-            fn simd_visitor(
-                &mut self,
-            ) -> Option<&mut dyn crate::VisitSimdOperator<'a, Output = Self::Output>> {
-                Some(self)
-            }
-
             crate::for_each_visit_operator!(define_visit_operator);
-        }
-
-        #[cfg(feature = "simd")]
-        impl<'a> VisitSimdOperator<'a> for VisitConstOperator<'a> {
             crate::for_each_visit_simd_operator!(define_visit_operator);
         }
     }

crates/wasmparser/src/validator/core.rs

@@ -176,17 +176,6 @@ impl<T: WasmModuleResources> FuncValidator<T> {
         self.validator.with_resources(&self.resources, offset)
     }
 
-    /// Same as [`FuncValidator::visitor`] except that the returned type
-    /// implements the [`VisitSimdOperator`](crate::VisitSimdOperator) trait as
-    /// well.
-    #[cfg(feature = "simd")]
-    pub fn simd_visitor<'this, 'a: 'this>(
-        &'this mut self,
-        offset: usize,
-    ) -> impl crate::VisitSimdOperator<'a, Output = Result<()>> + ModuleArity + 'this {
-        self.validator.with_resources_simd(&self.resources, offset)
-    }
-
     /// Returns the Wasm features enabled for this validator.
     pub fn features(&self) -> &WasmFeatures {
         &self.validator.features

crates/wasmparser/src/validator/func.rs

@@ -22,8 +22,6 @@
 // confusing it's recommended to read over that section to see how it maps to
 // the various methods here.
 
-#[cfg(feature = "simd")]
-use crate::VisitSimdOperator;
 use crate::{
     limits::MAX_WASM_FUNCTION_LOCALS, AbstractHeapType, BinaryReaderError, BlockType, BrTable,
     Catch, ContType, FieldType, FrameKind, FuncType, GlobalType, Handle, HeapType, Ieee32, Ieee64,
@@ -488,25 +486,6 @@ impl OperatorValidator {
         })
     }
 
-    /// Same as `with_resources` above but guarantees it's able to visit simd
-    /// operators as well.
-    #[cfg(feature = "simd")]
-    pub fn with_resources_simd<'a, 'validator, 'resources, T>(
-        &'validator mut self,
-        resources: &'resources T,
-        offset: usize,
-    ) -> impl VisitSimdOperator<'a, Output = Result<()>> + ModuleArity + 'validator
-    where
-        T: WasmModuleResources,
-        'resources: 'validator,
-    {
-        WasmProposalValidator(OperatorValidatorTemp {
-            offset,
-            inner: self,
-            resources,
-        })
-    }
-
     pub fn into_allocations(mut self) -> OperatorValidatorAllocations {
         fn clear<T>(mut tmp: Vec<T>) -> Vec<T> {
             tmp.clear();
@@ -998,6 +977,12 @@ where
         Ok(())
     }
 
+    #[cold]
+    #[cfg(not(feature = "simd"))]
+    fn simd_unavailable(&self) -> Result<()> {
+        bail!(self.offset, "SIMD not compiled-in");
+    }
+
     fn check_shared_memarg(&self, memarg: MemArg) -> Result<ValType> {
         if memarg.align != memarg.max_align {
             bail!(
@@ -1693,7 +1678,6 @@ impl<T> WasmProposalValidator<'_, '_, T> {
     }
 }
 
-#[cfg_attr(not(feature = "simd"), allow(unused_macro_rules))]
 macro_rules! validate_proposal {
     ($( @$proposal:ident $op:ident $({ $($arg:ident: $argty:ty),* })? => $visit:ident ($($ann:tt)*))*) => {
         $(
@@ -1732,21 +1716,7 @@ where
     T: WasmModuleResources,
 {
     type Output = Result<()>;
-
-    #[cfg(feature = "simd")]
-    fn simd_visitor(&mut self) -> Option<&mut dyn VisitSimdOperator<'a, Output = Self::Output>> {
-        Some(self)
-    }
-
-    crate::for_each_visit_operator!(validate_proposal);
-}
-
-#[cfg(feature = "simd")]
-impl<'a, T> VisitSimdOperator<'a> for WasmProposalValidator<'_, '_, T>
-where
-    T: WasmModuleResources,
-{
-    crate::for_each_visit_simd_operator!(validate_proposal);
+    crate::for_each_operator!(validate_proposal);
 }
 
 #[track_caller]
@@ -1764,17 +1734,29 @@ fn debug_assert_type_indices_are_ids(ty: ValType) {
     }
 }
 
+macro_rules! delegate_to_simd {
+    ($( @$proposal:ident $op:ident $({ $($arg:ident: $argty:ty),* })? => $visit:ident ($($ann:tt)*))*) => {
+        $(
+            #[cfg(feature = "simd")]
+            fn $visit(&mut self $($(,$arg: $argty)*)?) -> Result<()> {
+                simd::SimdOperatorValidatorTemp(self).$visit($( $($arg),* )?)
+            }
+
+            #[cfg(not(feature = "simd"))]
+            #[allow(unused_variables)]
+            fn $visit(&mut self $($(,$arg: $argty)*)?) -> Result<()> {
+                self.simd_unavailable()
+            }
+        )*
+    };
+}
+
 impl<'a, T> VisitOperator<'a> for OperatorValidatorTemp<'_, '_, T>
 where
     T: WasmModuleResources,
 {
     type Output = Result<()>;
 
-    #[cfg(feature = "simd")]
-    fn simd_visitor(&mut self) -> Option<&mut dyn VisitSimdOperator<'a, Output = Self::Output>> {
-        Some(self)
-    }
-
     fn visit_nop(&mut self) -> Self::Output {
         Ok(())
     }
@@ -4197,6 +4179,8 @@ where
     fn visit_i64_mul_wide_u(&mut self) -> Result<()> {
         self.check_i64_mul_wide()
     }
+
+    crate::for_each_visit_simd_operator!(delegate_to_simd);
 }
 
 #[derive(Clone, Debug)]