move the burden of evaluating override-expressions to users of naga's API

teoxoy · teoxoy · commit 7df0aa6364ed · 2024-04-05T18:07:41.000+02:00
diff --git a/naga-cli/src/bin/naga.rs b/naga-cli/src/bin/naga.rs
@@ -597,17 +597,18 @@ fn write_output(
             let mut options = params.msl.clone();
             options.bounds_check_policies = params.bounds_check_policies;
 
+            let info = info.as_ref().ok_or(CliError(
+                "Generating metal output requires validation to \
+                 succeed, and it failed in a previous step",
+            ))?;
+
+            let (module, info) =
+                naga::back::pipeline_constants::process_overrides(module, info, &params.overrides)
+                    .unwrap_pretty();
+
             let pipeline_options = msl::PipelineOptions::default();
-            let (msl, _) = msl::write_string(
-                module,
-                info.as_ref().ok_or(CliError(
-                    "Generating metal output requires validation to \
-                     succeed, and it failed in a previous step",
-                ))?,
-                &options,
-                &pipeline_options,
-            )
-            .unwrap_pretty();
+            let (msl, _) =
+                msl::write_string(&module, &info, &options, &pipeline_options).unwrap_pretty();
             fs::write(output_path, msl)?;
         }
         "spv" => {
@@ -624,23 +625,23 @@ fn write_output(
                     pipeline_options_owned = spv::PipelineOptions {
                         entry_point: name.clone(),
                         shader_stage: module.entry_points[ep_index].stage,
-                        constants: naga::back::PipelineConstants::default(),
                     };
                     Some(&pipeline_options_owned)
                 }
                 None => None,
             };
 
-            let spv = spv::write_vec(
-                module,
-                info.as_ref().ok_or(CliError(
-                    "Generating SPIR-V output requires validation to \
-                     succeed, and it failed in a previous step",
-                ))?,
-                &params.spv_out,
-                pipeline_options,
-            )
-            .unwrap_pretty();
+            let info = info.as_ref().ok_or(CliError(
+                "Generating SPIR-V output requires validation to \
+                 succeed, and it failed in a previous step",
+            ))?;
+
+            let (module, info) =
+                naga::back::pipeline_constants::process_overrides(module, info, &params.overrides)
+                    .unwrap_pretty();
+
+            let spv =
+                spv::write_vec(&module, &info, &params.spv_out, pipeline_options).unwrap_pretty();
             let bytes = spv
                 .iter()
                 .fold(Vec::with_capacity(spv.len() * 4), |mut v, w| {
@@ -665,17 +666,22 @@ fn write_output(
                     _ => unreachable!(),
                 },
                 multiview: None,
-                constants: naga::back::PipelineConstants::default(),
             };
 
+            let info = info.as_ref().ok_or(CliError(
+                "Generating glsl output requires validation to \
+                 succeed, and it failed in a previous step",
+            ))?;
+
+            let (module, info) =
+                naga::back::pipeline_constants::process_overrides(module, info, &params.overrides)
+                    .unwrap_pretty();
+
             let mut buffer = String::new();
             let mut writer = glsl::Writer::new(
                 &mut buffer,
-                module,
-                info.as_ref().ok_or(CliError(
-                    "Generating glsl output requires validation to \
-                     succeed, and it failed in a previous step",
-                ))?,
+                &module,
+                &info,
                 &params.glsl,
                 &pipeline_options,
                 params.bounds_check_policies,
@@ -692,20 +698,19 @@ fn write_output(
         }
         "hlsl" => {
             use naga::back::hlsl;
+
+            let info = info.as_ref().ok_or(CliError(
+                "Generating hlsl output requires validation to \
+                 succeed, and it failed in a previous step",
+            ))?;
+
+            let (module, info) =
+                naga::back::pipeline_constants::process_overrides(module, info, &params.overrides)
+                    .unwrap_pretty();
+
             let mut buffer = String::new();
             let mut writer = hlsl::Writer::new(&mut buffer, &params.hlsl);
-            writer
-                .write(
-                    module,
-                    info.as_ref().ok_or(CliError(
-                        "Generating hlsl output requires validation to \
-                         succeed, and it failed in a previous step",
-                    ))?,
-                    &hlsl::PipelineOptions {
-                        constants: params.overrides.clone(),
-                    },
-                )
-                .unwrap_pretty();
+            writer.write(&module, &info).unwrap_pretty();
             fs::write(output_path, buffer)?;
         }
         "wgsl" => {
diff --git a/naga/benches/criterion.rs b/naga/benches/criterion.rs
@@ -193,7 +193,6 @@ fn backends(c: &mut Criterion) {
                     let pipeline_options = naga::back::spv::PipelineOptions {
                         shader_stage: ep.stage,
                         entry_point: ep.name.clone(),
-                        constants: naga::back::PipelineConstants::default(),
                     };
                     writer
                         .write(module, info, Some(&pipeline_options), &None, &mut data)
@@ -224,11 +223,10 @@ fn backends(c: &mut Criterion) {
     group.bench_function("hlsl", |b| {
         b.iter(|| {
             let options = naga::back::hlsl::Options::default();
-            let pipeline_options = naga::back::hlsl::PipelineOptions::default();
             let mut string = String::new();
             for &(ref module, ref info) in inputs.iter() {
                 let mut writer = naga::back::hlsl::Writer::new(&mut string, &options);
-                let _ = writer.write(module, info, &pipeline_options); // may fail on unimplemented things
+                let _ = writer.write(module, info); // may fail on unimplemented things
                 string.clear();
             }
         });
@@ -250,7 +248,6 @@ fn backends(c: &mut Criterion) {
                         shader_stage: ep.stage,
                         entry_point: ep.name.clone(),
                         multiview: None,
-                        constants: naga::back::PipelineConstants::default(),
                     };
 
                     // might be `Err` if missing features
diff --git a/naga/src/back/glsl/mod.rs b/naga/src/back/glsl/mod.rs
@@ -294,8 +294,6 @@ pub struct PipelineOptions {
     pub entry_point: String,
     /// How many views to render to, if doing multiview rendering.
     pub multiview: Option<std::num::NonZeroU32>,
-    /// Pipeline constants.
-    pub constants: back::PipelineConstants,
 }
 
 #[derive(Debug)]
@@ -499,6 +497,8 @@ pub enum Error {
     ImageMultipleSamplers,
     #[error("{0}")]
     Custom(String),
+    #[error("overrides should not be present at this stage")]
+    Override,
 }
 
 /// Binary operation with a different logic on the GLSL side.
@@ -568,9 +568,7 @@ impl<'a, W: Write> Writer<'a, W> {
         policies: proc::BoundsCheckPolicies,
     ) -> Result<Self, Error> {
         if !module.overrides.is_empty() {
-            return Err(Error::Custom(
-                "Pipeline constants are not yet supported for this back-end".to_string(),
-            ));
+            return Err(Error::Override);
         }
 
         // Check if the requested version is supported
@@ -2544,7 +2542,7 @@ impl<'a, W: Write> Writer<'a, W> {
                     |writer, expr| writer.write_expr(expr, ctx),
                 )?;
             }
-            Expression::Override(_) => return Err(Error::Custom("overrides are WIP".into())),
+            Expression::Override(_) => return Err(Error::Override),
             // `Access` is applied to arrays, vectors and matrices and is written as indexing
             Expression::Access { base, index } => {
                 self.write_expr(base, ctx)?;
diff --git a/naga/src/back/hlsl/mod.rs b/naga/src/back/hlsl/mod.rs
@@ -195,14 +195,6 @@ pub struct Options {
     pub zero_initialize_workgroup_memory: bool,
 }
 
-#[derive(Clone, Debug, Default)]
-#[cfg_attr(feature = "serialize", derive(serde::Serialize))]
-#[cfg_attr(feature = "deserialize", derive(serde::Deserialize))]
-pub struct PipelineOptions {
-    /// Pipeline constants.
-    pub constants: back::PipelineConstants,
-}
-
 impl Default for Options {
     fn default() -> Self {
         Options {
@@ -255,8 +247,8 @@ pub enum Error {
     Unimplemented(String), // TODO: Error used only during development
     #[error("{0}")]
     Custom(String),
-    #[error(transparent)]
-    PipelineConstant(#[from] Box<back::pipeline_constants::PipelineConstantError>),
+    #[error("overrides should not be present at this stage")]
+    Override,
 }
 
 #[derive(Default)]
diff --git a/naga/src/back/hlsl/writer.rs b/naga/src/back/hlsl/writer.rs
@@ -1,7 +1,7 @@
 use super::{
     help::{WrappedArrayLength, WrappedConstructor, WrappedImageQuery, WrappedStructMatrixAccess},
     storage::StoreValue,
-    BackendResult, Error, Options, PipelineOptions,
+    BackendResult, Error, Options,
 };
 use crate::{
     back,
@@ -167,16 +167,10 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
         &mut self,
         module: &Module,
         module_info: &valid::ModuleInfo,
-        pipeline_options: &PipelineOptions,
     ) -> Result<super::ReflectionInfo, Error> {
-        let (module, module_info) = back::pipeline_constants::process_overrides(
-            module,
-            module_info,
-            &pipeline_options.constants,
-        )
-        .map_err(Box::new)?;
-        let module = module.as_ref();
-        let module_info = module_info.as_ref();
+        if !module.overrides.is_empty() {
+            return Err(Error::Override);
+        }
 
         self.reset(module);
 
@@ -2150,9 +2144,7 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
                     |writer, expr| writer.write_expr(module, expr, func_ctx),
                 )?;
             }
-            Expression::Override(_) => {
-                return Err(Error::Unimplemented("overrides are WIP".into()))
-            }
+            Expression::Override(_) => return Err(Error::Override),
             // All of the multiplication can be expressed as `mul`,
             // except vector * vector, which needs to use the "*" operator.
             Expression::Binary {
diff --git a/naga/src/back/mod.rs b/naga/src/back/mod.rs
@@ -22,7 +22,7 @@ pub mod wgsl;
     feature = "spv-out",
     feature = "glsl-out"
 ))]
-mod pipeline_constants;
+pub mod pipeline_constants;
 
 /// Names of vector components.
 pub const COMPONENTS: &[char] = &['x', 'y', 'z', 'w'];
diff --git a/naga/src/back/msl/mod.rs b/naga/src/back/msl/mod.rs
@@ -143,8 +143,8 @@ pub enum Error {
     UnsupportedArrayOfType(Handle<crate::Type>),
     #[error("ray tracing is not supported prior to MSL 2.3")]
     UnsupportedRayTracing,
-    #[error(transparent)]
-    PipelineConstant(#[from] Box<crate::back::pipeline_constants::PipelineConstantError>),
+    #[error("overrides should not be present at this stage")]
+    Override,
 }
 
 #[derive(Clone, Debug, PartialEq, thiserror::Error)]
@@ -234,8 +234,6 @@ pub struct PipelineOptions {
     ///
     /// Enable this for vertex shaders with point primitive topologies.
     pub allow_and_force_point_size: bool,
-    /// Pipeline constants.
-    pub constants: crate::back::PipelineConstants,
 }
 
 impl Options {
diff --git a/naga/src/back/msl/writer.rs b/naga/src/back/msl/writer.rs
@@ -1431,9 +1431,7 @@ impl<W: Write> Writer<W> {
                     |writer, context, expr| writer.put_expression(expr, context, true),
                 )?;
             }
-            crate::Expression::Override(_) => {
-                return Err(Error::FeatureNotImplemented("overrides are WIP".into()))
-            }
+            crate::Expression::Override(_) => return Err(Error::Override),
             crate::Expression::Access { base, .. }
             | crate::Expression::AccessIndex { base, .. } => {
                 // This is an acceptable place to generate a `ReadZeroSkipWrite` check.
@@ -3223,11 +3221,9 @@ impl<W: Write> Writer<W> {
         options: &Options,
         pipeline_options: &PipelineOptions,
     ) -> Result<TranslationInfo, Error> {
-        let (module, info) =
-            back::pipeline_constants::process_overrides(module, info, &pipeline_options.constants)
-                .map_err(Box::new)?;
-        let module = module.as_ref();
-        let info = info.as_ref();
+        if !module.overrides.is_empty() {
+            return Err(Error::Override);
+        }
 
         self.names.clear();
         self.namer.reset(
diff --git a/naga/src/back/pipeline_constants.rs b/naga/src/back/pipeline_constants.rs
@@ -36,7 +36,7 @@ pub enum PipelineConstantError {
 /// fully-evaluated expressions.
 ///
 /// [`global_expressions`]: Module::global_expressions
-pub(super) fn process_overrides<'a>(
+pub fn process_overrides<'a>(
     module: &'a Module,
     module_info: &'a ModuleInfo,
     pipeline_constants: &PipelineConstants,
diff --git a/naga/src/back/spv/block.rs b/naga/src/back/spv/block.rs
@@ -239,9 +239,7 @@ impl<'w> BlockContext<'w> {
                 let init = self.ir_module.constants[handle].init;
                 self.writer.constant_ids[init.index()]
             }
-            crate::Expression::Override(_) => {
-                return Err(Error::FeatureNotImplemented("overrides are WIP"))
-            }
+            crate::Expression::Override(_) => return Err(Error::Override),
             crate::Expression::ZeroValue(_) => self.writer.get_constant_null(result_type_id),
             crate::Expression::Compose { ty, ref components } => {
                 self.temp_list.clear();
diff --git a/naga/src/back/spv/mod.rs b/naga/src/back/spv/mod.rs
@@ -70,8 +70,8 @@ pub enum Error {
     FeatureNotImplemented(&'static str),
     #[error("module is not validated properly: {0}")]
     Validation(&'static str),
-    #[error(transparent)]
-    PipelineConstant(#[from] Box<crate::back::pipeline_constants::PipelineConstantError>),
+    #[error("overrides should not be present at this stage")]
+    Override,
 }
 
 #[derive(Default)]
@@ -773,8 +773,6 @@ pub struct PipelineOptions {
     ///
     /// If no entry point that matches is found while creating a [`Writer`], a error will be thrown.
     pub entry_point: String,
-    /// Pipeline constants.
-    pub constants: crate::back::PipelineConstants,
 }
 
 pub fn write_vec(
diff --git a/naga/src/back/spv/writer.rs b/naga/src/back/spv/writer.rs
@@ -2029,21 +2029,9 @@ impl Writer {
         debug_info: &Option<DebugInfo>,
         words: &mut Vec<Word>,
     ) -> Result<(), Error> {
-        let (ir_module, info) = if let Some(pipeline_options) = pipeline_options {
-            crate::back::pipeline_constants::process_overrides(
-                ir_module,
-                info,
-                &pipeline_options.constants,
-            )
-            .map_err(Box::new)?
-        } else {
-            (
-                std::borrow::Cow::Borrowed(ir_module),
-                std::borrow::Cow::Borrowed(info),
-            )
-        };
-        let ir_module = ir_module.as_ref();
-        let info = info.as_ref();
+        if !ir_module.overrides.is_empty() {
+            return Err(Error::Override);
+        }
 
         self.reset();
 
diff --git a/naga/src/back/wgsl/writer.rs b/naga/src/back/wgsl/writer.rs
@@ -1205,9 +1205,7 @@ impl<W: Write> Writer<W> {
                     |writer, expr| writer.write_expr(module, expr, func_ctx),
                 )?;
             }
-            Expression::Override(_) => {
-                return Err(Error::Unimplemented("overrides are WIP".into()))
-            }
+            Expression::Override(_) => unreachable!(),
             Expression::FunctionArgument(pos) => {
                 let name_key = func_ctx.argument_key(pos);
                 let name = &self.names[&name_key];
diff --git a/naga/tests/in/interface.param.ron b/naga/tests/in/interface.param.ron
@@ -27,6 +27,5 @@
 	),
 	msl_pipeline: (
 		allow_and_force_point_size: true,
-		constants: {},
 	),
 )
diff --git a/naga/tests/out/glsl/overrides.main.Compute.glsl b/naga/tests/out/glsl/overrides.main.Compute.glsl
diff --git a/naga/tests/snapshots.rs b/naga/tests/snapshots.rs
diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs
diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs
diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs
diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs
