From d44e2ad207883f47019316ccbd6f02c27b27cde9 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Wed, 22 Sep 2021 12:08:04 -0700 Subject: [PATCH] Simplify interpolation defaulting. Replace `Module::apply_common_default_interpolation` with a simpler function that handles a single `Binding` at a time. In exchange for the simplicity, the function must be called at each point function arguments, function results, and struct members are prepared. (Any missed spots will be caught by the verifier.) This approach no longer requires mutating types in the arena, a prerequisite for properly handling type identity. Applying defaults to struct members when the struct declaration is parsed does have a disadvantage, compared to the old whole-module pass: at struct parse time, we don't yet know which pipeline stages the struct will be used in. The best we can do is apply defaults to anything with a `Location` binding. This causes needless qualifiers to appear in some output. However, it seems that our back end languages all tolerate such qualifiers. --- src/front/interpolator.rs | 59 ++++++++++ src/front/mod.rs | 2 + src/front/spv/function.rs | 11 +- src/front/spv/mod.rs | 18 +++- src/front/wgsl/mod.rs | 19 +++- src/lib.rs | 16 +++ src/proc/interpolator.rs | 149 -------------------------- src/proc/mod.rs | 1 - src/valid/interface.rs | 4 - tests/out/hlsl/interface.hlsl | 2 +- tests/out/ir/shadow.ron | 4 +- tests/out/spv/interface.vertex.spvasm | 1 + 12 files changed, 111 insertions(+), 175 deletions(-) create mode 100644 src/front/interpolator.rs delete mode 100644 src/proc/interpolator.rs diff --git a/src/front/interpolator.rs b/src/front/interpolator.rs new file mode 100644 index 0000000000..491ca5a0bb --- /dev/null +++ b/src/front/interpolator.rs @@ -0,0 +1,59 @@ +//! Interpolation defaults. + +impl crate::Binding { + /// Apply the usual default interpolation for `ty` to `binding`. + /// + /// This function is a utility front ends may use to satisfy the Naga IR's + /// requirement, meant to ensure that input languages' policies have been + /// applied appropriately, that all I/O `Binding`s from the vertex shader to the + /// fragment shader must have non-`None` `interpolation` values. + /// + /// All the shader languages Naga supports have similar rules: + /// perspective-correct, center-sampled interpolation is the default for any + /// binding that can vary, and everything else either defaults to flat, or + /// requires an explicit flat qualifier/attribute/what-have-you. + /// + /// If `binding` is not a [`Location`] binding, or if its [`interpolation`] is + /// already set, then make no changes. Otherwise, set `binding`'s interpolation + /// and sampling to reasonable defaults depending on `ty`, the type of the value + /// being interpolated: + /// + /// - If `ty` is a floating-point scalar, vector, or matrix type, then + /// default to [`Perspective`] interpolation and [`Center`] sampling. + /// + /// - If `ty` is an integral scalar or vector, then default to [`Flat`] + /// interpolation, which has no associated sampling. + /// + /// - For any other types, make no change. Such types are not permitted as + /// user-defined IO values, and will probably be flagged by the verifier + /// + /// When structs appear in input or output types, each member ought to have its + /// own [`Binding`], so structs are simply covered by the third case. + /// + /// [`Binding`]: crate::Binding + /// [`Location`]: crate::Binding::Location + /// [`interpolation`]: crate::Binding::Location::interpolation + /// [`Perspective`]: crate::Interpolation::Perspective + /// [`Flat`]: crate::Interpolation::Flat + /// [`Center`]: crate::Sampling::Center + pub fn apply_default_interpolation(&mut self, ty: &crate::TypeInner) { + if let crate::Binding::Location { + location: _, + interpolation: ref mut interpolation @ None, + ref mut sampling, + } = *self + { + match ty.scalar_kind() { + Some(crate::ScalarKind::Float) => { + *interpolation = Some(crate::Interpolation::Perspective); + *sampling = Some(crate::Sampling::Center); + } + Some(crate::ScalarKind::Sint) | Some(crate::ScalarKind::Uint) => { + *interpolation = Some(crate::Interpolation::Flat); + *sampling = None; + } + Some(_) | None => {} + } + } + } +} diff --git a/src/front/mod.rs b/src/front/mod.rs index 292acf3da3..56e000d62d 100644 --- a/src/front/mod.rs +++ b/src/front/mod.rs @@ -1,5 +1,7 @@ //! Parsers which load shaders into memory. +mod interpolator; + #[cfg(feature = "glsl-in")] pub mod glsl; #[cfg(feature = "spv-in")] diff --git a/src/front/spv/function.rs b/src/front/spv/function.rs index 90ea81033e..0ce8e7686b 100644 --- a/src/front/spv/function.rs +++ b/src/front/spv/function.rs @@ -341,13 +341,8 @@ impl> super::Parser { let mut arg = arg.clone(); if ep.stage == crate::ShaderStage::Fragment { - if let Some(crate::Binding::Location { - interpolation: ref mut interpolation @ None, - .. - }) = arg.binding - { - *interpolation = Some(crate::Interpolation::Perspective); - // default + if let Some(ref mut binding) = arg.binding { + binding.apply_default_interpolation(&module.types[arg.ty].inner); } } function.arguments.push(arg); @@ -516,8 +511,6 @@ impl> super::Parser { }); } - module.apply_common_default_interpolation(); - Ok(()) } } diff --git a/src/front/spv/mod.rs b/src/front/spv/mod.rs index 4869a47868..34b510d355 100644 --- a/src/front/spv/mod.rs +++ b/src/front/spv/mod.rs @@ -3725,7 +3725,7 @@ impl> Parser { span = crate::front::align_up(span, self.layouter[ty].alignment.get()); alignment = self.layouter[ty].alignment.get().max(alignment); - let binding = decor.io_binding().ok(); + let mut binding = decor.io_binding().ok(); if let Some(offset) = decor.offset { span = offset; } @@ -3733,11 +3733,12 @@ impl> Parser { span += self.layouter[ty].size; + let inner = &module.types[ty].inner; if let crate::TypeInner::Matrix { columns, rows, width, - } = module.types[ty].inner + } = *inner { if let Some(stride) = decor.matrix_stride { let rounded_rows = if rows > crate::VectorSize::Bi { @@ -3757,6 +3758,9 @@ impl> Parser { } } + if let Some(ref mut binding) = binding { + binding.apply_default_interpolation(inner); + } members.push(crate::StructMember { name: decor.name, ty, @@ -4181,7 +4185,7 @@ impl> Parser { (Variable::Global, var) } ExtendedClass::Input => { - let binding = dec.io_binding()?; + let mut binding = dec.io_binding()?; let mut unsigned_ty = effective_ty; if let crate::Binding::BuiltIn(built_in) = binding { let needs_inner_uint = match built_in { @@ -4222,6 +4226,9 @@ impl> Parser { ty: effective_ty, init: None, }; + + binding.apply_default_interpolation(&module.types[unsigned_ty].inner); + let inner = Variable::Input(crate::FunctionArgument { name: dec.name, ty: unsigned_ty, @@ -4231,7 +4238,7 @@ impl> Parser { } ExtendedClass::Output => { // For output interface blocks, this would be a structure. - let binding = dec.io_binding().ok(); + let mut binding = dec.io_binding().ok(); let init = match binding { Some(crate::Binding::BuiltIn(built_in)) => { match null::generate_default_built_in( @@ -4297,6 +4304,9 @@ impl> Parser { ty: effective_ty, init, }; + if let Some(ref mut binding) = binding { + binding.apply_default_interpolation(&module.types[effective_ty].inner); + } let inner = Variable::Output(crate::FunctionResult { ty: effective_ty, binding, diff --git a/src/front/wgsl/mod.rs b/src/front/wgsl/mod.rs index bc013f1474..09c66bb63d 100644 --- a/src/front/wgsl/mod.rs +++ b/src/front/wgsl/mod.rs @@ -1085,7 +1085,7 @@ impl BindingParser { (None, None, None, None) => Ok(None), (Some(location), None, interpolation, sampling) => { // Before handing over the completed `Module`, we call - // `apply_common_default_interpolation` to ensure that the interpolation and + // `apply_default_interpolation` to ensure that the interpolation and // sampling have been explicitly specified on all vertex shader output and fragment // shader input user bindings, so leaving them potentially `None` here is fine. Ok(Some(crate::Binding::Location { @@ -2704,10 +2704,14 @@ impl Parser { alignment = alignment.max(align); offset = range.end; + let mut binding = bind_parser.finish(bind_span)?; + if let Some(ref mut binding) = binding { + binding.apply_default_interpolation(&type_arena[ty].inner); + } members.push(crate::StructMember { name: Some(name.to_owned()), ty, - binding: bind_parser.finish(bind_span)?, + binding, offset: range.start, }); } @@ -3818,7 +3822,7 @@ impl Parser { ExpectedToken::Token(Token::Separator(',')), )); } - let binding = self.parse_varying_binding(lexer)?; + let mut binding = self.parse_varying_binding(lexer)?; let (param_name, param_name_span, param_type, _access) = self.parse_variable_ident_decl(lexer, &mut module.types, &mut module.constants)?; let param_index = arguments.len() as u32; @@ -3833,6 +3837,9 @@ impl Parser { is_reference: false, }, ); + if let Some(ref mut binding) = binding { + binding.apply_default_interpolation(&module.types[param_type].inner); + } arguments.push(crate::FunctionArgument { name: Some(param_name.to_string()), ty: param_type, @@ -3842,9 +3849,12 @@ impl Parser { } // read return type let result = if lexer.skip(Token::Arrow) && !lexer.skip(Token::Word("void")) { - let binding = self.parse_varying_binding(lexer)?; + let mut binding = self.parse_varying_binding(lexer)?; let (ty, _access) = self.parse_type_decl(lexer, None, &mut module.types, &mut module.constants)?; + if let Some(ref mut binding) = binding { + binding.apply_default_interpolation(&module.types[ty].inner); + } Some(crate::FunctionResult { ty, binding }) } else { None @@ -4122,7 +4132,6 @@ impl Parser { log::error!("Reached the end of file, but scopes are not closed"); return Err(Error::Other.as_parse_error(lexer.source)); }; - module.apply_common_default_interpolation(); return Ok(module); } } diff --git a/src/lib.rs b/src/lib.rs index 697264c516..afc93094d0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -710,7 +710,23 @@ pub enum ConstantInner { pub enum Binding { /// Built-in shader variable. BuiltIn(BuiltIn), + /// Indexed location. + /// + /// Values passed from the [`Vertex`] stage to the [`Fragment`] stage must + /// have their `interpolation` defaulted (i.e. not `None`) by the front end + /// as appropriate for that language. + /// + /// For other stages, we permit interpolations even though they're ignored. + /// When a front end is parsing a struct type, it usually doesn't know what + /// stages will be using it for IO, so it's easiest if it can apply the + /// defaults to anything with a `Location` binding, just in case. + /// + /// For anything other than floating-point scalars and vectors, the + /// interpolation must be `Flat`. + /// + /// [`Vertex`]: crate::ShaderStage::Vertex + /// [`Fragment`]: crate::ShaderStage::Fragment Location { location: u32, interpolation: Option, diff --git a/src/proc/interpolator.rs b/src/proc/interpolator.rs deleted file mode 100644 index 3b550782c9..0000000000 --- a/src/proc/interpolator.rs +++ /dev/null @@ -1,149 +0,0 @@ -pub use crate::{Arena, Handle}; - -impl crate::Module { - /// Apply the usual default interpolation for vertex shader outputs and fragment shader inputs. - /// - /// For every [`Binding`] that is a vertex shader output or a fragment shader - /// input, and that has an `interpolation` or `sampling` of `None`, assign a - /// default interpolation and sampling as follows: - /// - /// - If the `Binding`'s type contains only 32-bit floating-point values or - /// vectors of such, default its interpolation to `Perspective` and its - /// sampling to `Center`. - /// - /// - Otherwise, mark its interpolation as `Flat`. - /// - /// When struct appear in input or output types, apply these rules to their - /// leaves, since those are the things that actually get assigned locations. - /// - /// This function is a utility front ends may use to satisfy the Naga IR's - /// requirement that all I/O `Binding`s from the vertex shader to the - /// fragment shader must have non-`None` `interpolation` values. This - /// requirement is meant to ensure that input languages' policies have been - /// applied appropriately. - /// - /// All the shader languages Naga supports have similar rules: - /// perspective-correct, center-sampled interpolation is the default for any - /// binding that can vary, and everything else either defaults to flat, or - /// requires an explicit flat qualifier/attribute/what-have-you. - /// - /// [`Binding`]: crate::Binding - pub fn apply_common_default_interpolation(&mut self) { - use crate::{Binding, ScalarKind, Type, TypeInner}; - - /// Choose a default interpolation for a function argument or result. - /// - /// `binding` refers to the `Binding` whose type is `ty`. If `ty` is a struct, then it's the - /// bindings of the struct's members that we care about, and the binding of the struct - /// itself is meaningless, so `binding` should be `None`. - fn default_binding_or_struct( - binding: &mut Option, - ty: Handle, - types: &mut Arena, - ) { - let inner = &mut types.get_mut(ty).inner; - if let TypeInner::Struct { - members: ref mut m, .. - } = *inner - { - // A struct. It's the individual members we care about, so recurse. - - // To choose the right interpolations for `members`, we must consult other - // elements of `types`. But both `members` and the types it refers to are stored - // in `types`, and Rust won't let us mutate one element of the `Arena`'s `Vec` - // while reading others. - // - // So, temporarily swap the member list out its type, assign appropriate - // interpolations to its members, and then swap the list back in. - use std::mem; - let mut members = mem::take(m); - - for member in &mut members { - default_binding_or_struct(&mut member.binding, member.ty, types); - } - - // Swap the member list back in. It's essential that we call `types.get_mut` - // afresh here, rather than just using `m`: it's only because `m` was dead that - // we were able to pass `types` to the recursive call. - match types.get_mut(ty).inner { - TypeInner::Struct { - members: ref mut m, .. - } => mem::replace(m, members), - _ => unreachable!("ty must be a struct"), - }; - - return; - } - - // For all other types, a binding is required. Missing bindings will - // be caught during validation, but this processor is meant for use - // by front ends before validation, so just return for now. - let binding = match binding.as_mut() { - None => return, - Some(binding) => binding, - }; - - match *inner { - // Some interpolatable type. - // - // GLSL has 64-bit floats, but it won't interpolate them. WGSL and MSL only have - // 32-bit floats. SPIR-V has 16- and 64-bit float capabilities, but Vulkan is vague - // about what can and cannot be interpolated. - TypeInner::Scalar { - kind: ScalarKind::Float, - width: 4, - } - | TypeInner::Vector { - kind: ScalarKind::Float, - width: 4, - .. - } => { - if let Binding::Location { - ref mut interpolation, - ref mut sampling, - .. - } = *binding - { - if interpolation.is_none() { - *interpolation = Some(crate::Interpolation::Perspective); - } - if sampling.is_none() && *interpolation != Some(crate::Interpolation::Flat) - { - *sampling = Some(crate::Sampling::Center); - } - } - } - - // Some type that can't be interpolated. - _ => { - if let Binding::Location { - ref mut interpolation, - ref mut sampling, - .. - } = *binding - { - *interpolation = Some(crate::Interpolation::Flat); - *sampling = None; - } - } - } - } - - for ep in &mut self.entry_points { - let function = &mut ep.function; - match ep.stage { - crate::ShaderStage::Fragment => { - for arg in &mut function.arguments { - default_binding_or_struct(&mut arg.binding, arg.ty, &mut self.types); - } - } - crate::ShaderStage::Vertex => { - if let Some(result) = function.result.as_mut() { - default_binding_or_struct(&mut result.binding, result.ty, &mut self.types); - } - } - _ => (), - } - } - } -} diff --git a/src/proc/mod.rs b/src/proc/mod.rs index 867782ffe7..d672e905c4 100644 --- a/src/proc/mod.rs +++ b/src/proc/mod.rs @@ -1,7 +1,6 @@ //! Module processing functionality. mod index; -mod interpolator; mod layouter; mod namer; mod terminator; diff --git a/src/valid/interface.rs b/src/valid/interface.rs index 9253042af8..ff3f2401cd 100644 --- a/src/valid/interface.rs +++ b/src/valid/interface.rs @@ -256,10 +256,6 @@ impl VaryingContext<'_> { return Err(VaryingError::BindingCollision { location }); } - // Values passed from the vertex shader to the fragment shader must have their - // interpolation defaulted (i.e. not `None`) by the front end, as appropriate for - // that language. For anything other than floating-point scalars and vectors, the - // interpolation must be `Flat`. let needs_interpolation = match self.stage { crate::ShaderStage::Vertex => self.output, crate::ShaderStage::Fragment => !self.output, diff --git a/tests/out/hlsl/interface.hlsl b/tests/out/hlsl/interface.hlsl index affd807ecd..3c4334defb 100644 --- a/tests/out/hlsl/interface.hlsl +++ b/tests/out/hlsl/interface.hlsl @@ -13,7 +13,7 @@ struct VertexOutput { struct FragmentOutput { float depth : SV_Depth; uint sample_mask : SV_Coverage; - float color : SV_Target0; + linear float color : SV_Target0; }; groupshared uint output[1]; diff --git a/tests/out/ir/shadow.ron b/tests/out/ir/shadow.ron index 8c4e2df9ed..763aa28e9c 100644 --- a/tests/out/ir/shadow.ron +++ b/tests/out/ir/shadow.ron @@ -1527,8 +1527,8 @@ ty: 4, binding: Some(Location( location: 0, - interpolation: None, - sampling: None, + interpolation: Some(Perspective), + sampling: Some(Center), )), )), local_variables: [], diff --git a/tests/out/spv/interface.vertex.spvasm b/tests/out/spv/interface.vertex.spvasm index 9910b2bc5a..c51a4d563b 100644 --- a/tests/out/spv/interface.vertex.spvasm +++ b/tests/out/spv/interface.vertex.spvasm @@ -15,6 +15,7 @@ OpDecorate %15 ArrayStride 4 OpDecorate %18 BuiltIn VertexIndex OpDecorate %21 BuiltIn InstanceIndex OpDecorate %23 Location 10 +OpDecorate %23 Flat OpDecorate %25 BuiltIn Position OpDecorate %27 Location 1 OpDecorate %29 BuiltIn PointSize