Repository: gfx-rs/wgpu
Pull request #6334 (2024-09-27 04:23:36)

[naga spv-out] Document and refactor `write_runtime_array_length`.
* Document and refactor `naga::back::spv::BlockContext::write_runtime_array_length`.

  Don't try to handle finding the length of a particular element of a `binding_array<array<T>>`. The SPIR-V backend doesn't wrap that type correctly anyway; #6333 changes the validator to forbid such types. Instead, assume that the elements of a `binding_array<T>` are always structs whose final members may be a runtime-sized array.

  Pull out consistency checks after the analysis of the array expression, so that we always carry out all the checks regardless of what path we took to produce the information.

* Update Vulkan spec section number, and provide link.

* Replace `match` with equivalent `!=`.



Patch:
diff --git a/naga/src/back/spv/block.rs b/naga/src/back/spv/block.rs
index 9fb948586..f0c3bfa84 100644
--- a/naga/src/back/spv/block.rs
+++ b/naga/src/back/spv/block.rs
@@ -200,10 +200,7 @@ impl<'w> BlockContext<'w> {
     fn is_intermediate(&self, expr_handle: Handle<crate::Expression>) -> bool {
         match self.ir_function.expressions[expr_handle] {
             crate::Expression::GlobalVariable(handle) => {
-                match self.ir_module.global_variables[handle].space {
-                    crate::AddressSpace::Handle => false,
-                    _ => true,
-                }
+                self.ir_module.global_variables[handle].space != crate::AddressSpace::Handle
             }
             crate::Expression::LocalVariable(_) => true,
             crate::Expression::FunctionArgument(index) => {
diff --git a/naga/src/back/spv/helpers.rs b/naga/src/back/spv/helpers.rs
index 1fb447e38..15c241d44 100644
--- a/naga/src/back/spv/helpers.rs
+++ b/naga/src/back/spv/helpers.rs
@@ -85,7 +85,7 @@ impl crate::AddressSpace {
 
 /// Return true if the global requires a type decorated with `Block`.
 ///
-/// Vulkan spec v1.3 §15.6.2, "Descriptor Set Interface", says:
+/// In the Vulkan spec 1.3.296, the section [Descriptor Set Interface][dsi] says:
 ///
 /// > Variables identified with the `Uniform` storage class are used to
 /// > access transparent buffer backed resources. Such variables must
@@ -98,6 +98,8 @@ impl crate::AddressSpace {
 /// > -   laid out explicitly using the `Offset`, `ArrayStride`, and
 /// >     `MatrixStride` decorations as specified in §15.6.4, "Offset
 /// >     and Stride Assignment."
+///
+/// [dsi]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#interfaces-resources-descset
 // See `back::spv::GlobalVariable::access_id` for details.
 pub fn global_needs_wrapper(ir_module: &crate::Module, var: &crate::GlobalVariable) -> bool {
     match var.space {
diff --git a/naga/src/back/spv/index.rs b/naga/src/back/spv/index.rs
index 84b81b63d..0295d895b 100644
--- a/naga/src/back/spv/index.rs
+++ b/naga/src/back/spv/index.rs
@@ -38,98 +38,162 @@ impl<'w> BlockContext<'w> {
     ///
     /// Given `array`, an expression referring a runtime-sized array, return the
     /// instruction id for the array's length.
+    ///
+    /// Runtime-sized arrays may only appear in the values of global
+    /// variables, which must have one of the following Naga types:
+    ///
+    /// 1. A runtime-sized array.
+    /// 2. A struct whose last member is a runtime-sized array.
+    /// 3. A binding array of 2.
+    ///
+    /// Thus, the expression `array` has the form of:
+    ///
+    /// - An optional [`AccessIndex`], for case 2, applied to...
+    /// - An optional [`Access`] or [`AccessIndex`], for case 3, applied to...
+    /// - A [`GlobalVariable`].
+    ///
+    /// The SPIR-V generated takes into account wrapped globals; see
+    /// [`global_needs_wrapper`].
+    ///
+    /// [`GlobalVariable`]: crate::Expression::GlobalVariable
+    /// [`AccessIndex`]: crate::Expression::AccessIndex
+    /// [`Access`]: crate::Expression::Access
+    /// [`base`]: crate::Expression::Access::base
     pub(super) fn write_runtime_array_length(
         &mut self,
         array: Handle<crate::Expression>,
         block: &mut Block,
     ) -> Result<Word, Error> {
-        // Naga IR permits runtime-sized arrays as global variables, or as the
-        // final member of a struct that is a global variable, or one of these
-        // inside a buffer that is itself an element in a buffer bindings array.
-        // SPIR-V requires that runtime-sized arrays are wrapped in structs.
-        // See `helpers::global_needs_wrapper` and its uses.
-        let (opt_array_index_id, global_handle, opt_last_member_index) = match self
-            .ir_function
-            .expressions[array]
-        {
+        // The index into the binding array, if any.
+        let binding_array_index_id: Option<Word>;
+
+        // The handle to the Naga IR global we're referring to.
+        let global_handle: Handle<crate::GlobalVariable>;
+
+        // At the Naga type level, if the runtime-sized array is the final member of a
+        // struct, this is that member's index.
+        //
+        // This does not cover wrappers: if this backend wrapped the Naga global's
+        // type in a synthetic SPIR-V struct (see `global_needs_wrapper`), this is
+        // `None`.
+        let opt_last_member_index: Option<u32>;
+
+        // Inspect `array` and decide whether we have a binding array and/or an
+        // enclosing struct.
+        match self.ir_function.expressions[array] {
             crate::Expression::AccessIndex { base, index } => {
                 match self.ir_function.expressions[base] {
-                    // The global variable is an array of buffer bindings of structs,
-                    // we are accessing one of them with a static index,
-                    // and the last member of it.
                     crate::Expression::AccessIndex {
                         base: base_outer,
                         index: index_outer,
                     } => match self.ir_function.expressions[base_outer] {
+                        // An `AccessIndex` of an `AccessIndex` must be a
+                        // binding array holding structs whose last members are
+                        // runtime-sized arrays.
                         crate::Expression::GlobalVariable(handle) => {
                             let index_id = self.get_index_constant(index_outer);
-                            (Some(index_id), handle, Some(index))
+                            binding_array_index_id = Some(index_id);
+                            global_handle = handle;
+                            opt_last_member_index = Some(index);
+                        }
+                        _ => {
+                            return Err(Error::Validation(
+                                "array length expression: AccessIndex(AccessIndex(Global))",
+                            ))
                         }
-                        _ => return Err(Error::Validation("array length expression case-1a")),
                     },
-                    // The global variable is an array of buffer bindings of structs,
-                    // we are accessing one of them with a dynamic index,
-                    // and the last member of it.
                     crate::Expression::Access {
                         base: base_outer,
                         index: index_outer,
                     } => match self.ir_function.expressions[base_outer] {
+                        // Similarly, an `AccessIndex` of an `Access` must be a
+                        // binding array holding structs whose last members are
+                        // runtime-sized arrays.
                         crate::Expression::GlobalVariable(handle) => {
                             let index_id = self.cached[index_outer];
-                            (Some(index_id), handle, Some(index))
+                            binding_array_index_id = Some(index_id);
+                            global_handle = handle;
+                            opt_last_member_index = Some(index);
+                        }
+                        _ => {
+                            return Err(Error::Validation(
+                                "array length expression: AccessIndex(Access(Global))",
+                            ))
                         }
-                        _ => return Err(Error::Validation("array length expression case-1b")),
                     },
-                    // The global variable is a buffer, and we are accessing the last member.
                     crate::Expression::GlobalVariable(handle) => {
-                        let global = &self.ir_module.global_variables[handle];
-                        match self.ir_module.types[global.ty].inner {
-                            // The global variable is an array of buffer bindings of run-time arrays.
-                            crate::TypeInner::BindingArray { .. } => (Some(index), handle, None),
-                            // The global variable is a struct, and we are accessing the last member
-                            _ => (None, handle, Some(index)),
-                        }
+                        // An outer `AccessIndex` applied directly to a
+                        // `GlobalVariable`. Since binding arrays can only contain
+                        // structs, this must be referring to the last member of a
+                        // struct that is a runtime-sized array.
+                        binding_array_index_id = None;
+                        global_handle = handle;
+                        opt_last_member_index = Some(index);
                     }
-                    _ => return Err(Error::Validation("array length expression case-1c")),
-                }
-            }
-            // The global variable is an array of buffer bindings of arrays.
-            crate::Expression::Access { base, index } => match self.ir_function.expressions[base] {
-                crate::Expression::GlobalVariable(handle) => {
-                    let index_id = self.cached[index];
-                    let global = &self.ir_module.global_variables[handle];
-                    match self.ir_module.types[global.ty].inner {
-                        crate::TypeInner::BindingArray { .. } => (Some(index_id), handle, None),
-                        _ => return Err(Error::Validation("array length expression case-2a")),
+                    _ => {
+                        return Err(Error::Validation(
+                            "array length expression: AccessIndex(<unexpected>)",
+                        ))
                     }
                 }
-                _ => return Err(Error::Validation("array length expression case-2b")),
-            },
-            // The global variable is a run-time array.
+            }
             crate::Expression::GlobalVariable(handle) => {
-                let global = &self.ir_module.global_variables[handle];
-                if !global_needs_wrapper(self.ir_module, global) {
-                    return Err(Error::Validation("array length expression case-3"));
-                }
-                (None, handle, None)
+                // A direct reference to a global variable. This must hold the
+                // runtime-sized array directly.
+                binding_array_index_id = None;
+                global_handle = handle;
+                opt_last_member_index = None;
             }
             _ => return Err(Error::Validation("array length expression case-4")),
         };
 
+        // The verifier should have checked this, but make sure the inspection above
+        // agrees with the type about whether a binding array is involved.
+        //
+        // Eventually we do want to support `binding_array<array<T>>`. This check
+        // ensures that whoever relaxes the validator will get an error message from
+        // us, not just bogus SPIR-V.
+        let global = &self.ir_module.global_variables[global_handle];
+        match (
+            &self.ir_module.types[global.ty].inner,
+            binding_array_index_id,
+        ) {
+            (&crate::TypeInner::BindingArray { .. }, Some(_)) => {}
+            (_, None) => {}
+            _ => {
+                return Err(Error::Validation(
+                    "array length expression: bad binding array inference",
+                ))
+            }
+        }
+
+        // SPIR-V allows runtime-sized arrays to appear only as the last member of a
+        // struct. Determine this member's index.
         let gvar = self.writer.global_variables[global_handle].clone();
         let global = &self.ir_module.global_variables[global_handle];
-        let (last_member_index, gvar_id) = match opt_last_member_index {
-            Some(index) => (index, gvar.access_id),
-            None => {
-                if !global_needs_wrapper(self.ir_module, global) {
-                    return Err(Error::Validation(
-                        "pointer to a global that is not a wrapped array",
-                    ));
-                }
+        let needs_wrapper = global_needs_wrapper(self.ir_module, global);
+        let (last_member_index, gvar_id) = match (opt_last_member_index, needs_wrapper) {
+            (Some(index), false) => {
+                // At the Naga type level, the runtime-sized array appears as the
+                // final member of a struct, whose index is `index`. We didn't need to
+                // wrap this, since the Naga type meets SPIR-V's requirements already.
+                (index, gvar.access_id)
+            }
+            (None, true) => {
+                // At the Naga type level, the runtime-sized array does not appear
+                // within a struct. We wrapped this in an OpTypeStruct with nothing
+                // else in it, so the index is zero. OpArrayLength wants the pointer
+                // to the wrapper struct, so use `gvar.var_id`.
                 (0, gvar.var_id)
             }
+            _ => {
+                return Err(Error::Validation(
+                    "array length expression: bad SPIR-V wrapper struct inference",
+                ));
+            }
         };
-        let structure_id = match opt_array_index_id {
+
+        let structure_id = match binding_array_index_id {
             // We are indexing inside a binding array, generate the access op.
             Some(index_id) => {
                 let element_type_id = match self.ir_module.types[global.ty].inner {
diff --git a/naga/src/valid/type.rs b/naga/src/valid/type.rs
index 0c4466077..c0c25dab7 100644
--- a/naga/src/valid/type.rs
+++ b/naga/src/valid/type.rs
@@ -677,7 +677,6 @@ impl super::Validator {
                     // Currently Naga only supports binding arrays of structs for non-handle types.
                     match gctx.types[base].inner {
                         crate::TypeInner::Struct { .. } => {}
-                        crate::TypeInner::Array { .. } => {}
                         _ => return Err(TypeError::BindingArrayBaseTypeNotStruct(base)),
                     };
                 }


Test patch:
diff --git a/naga/tests/validation.rs b/naga/tests/validation.rs
index f20ae688b..b8a30c49b 100644
--- a/naga/tests/validation.rs
+++ b/naga/tests/validation.rs
@@ -496,3 +496,113 @@ fn main(input: VertexOutput) {{
         }
     }
 }
+
+#[allow(dead_code)]
+struct BindingArrayFixture {
+    module: naga::Module,
+    span: naga::Span,
+    ty_u32: naga::Handle<naga::Type>,
+    ty_array: naga::Handle<naga::Type>,
+    ty_struct: naga::Handle<naga::Type>,
+    validator: naga::valid::Validator,
+}
+
+impl BindingArrayFixture {
+    fn new() -> Self {
+        let mut module = naga::Module::default();
+        let span = naga::Span::default();
+        let ty_u32 = module.types.insert(
+            naga::Type {
+                name: Some("u32".into()),
+                inner: naga::TypeInner::Scalar(naga::Scalar::U32),
+            },
+            span,
+        );
+        let ty_array = module.types.insert(
+            naga::Type {
+                name: Some("array<u32, 10>".into()),
+                inner: naga::TypeInner::Array {
+                    base: ty_u32,
+                    size: naga::ArraySize::Constant(std::num::NonZeroU32::new(10).unwrap()),
+                    stride: 4,
+                },
+            },
+            span,
+        );
+        let ty_struct = module.types.insert(
+            naga::Type {
+                name: Some("S".into()),
+                inner: naga::TypeInner::Struct {
+                    members: vec![naga::StructMember {
+                        name: Some("m".into()),
+                        ty: ty_u32,
+                        binding: None,
+                        offset: 0,
+                    }],
+                    span: 4,
+                },
+            },
+            span,
+        );
+        let validator = naga::valid::Validator::new(Default::default(), Default::default());
+        BindingArrayFixture {
+            module,
+            span,
+            ty_u32,
+            ty_array,
+            ty_struct,
+            validator,
+        }
+    }
+}
+
+#[test]
+fn binding_arrays_hold_structs() {
+    let mut t = BindingArrayFixture::new();
+    let _binding_array = t.module.types.insert(
+        naga::Type {
+            name: Some("binding_array_of_struct".into()),
+            inner: naga::TypeInner::BindingArray {
+                base: t.ty_struct,
+                size: naga::ArraySize::Dynamic,
+            },
+        },
+        t.span,
+    );
+
+    assert!(t.validator.validate(&t.module).is_ok());
+}
+
+#[test]
+fn binding_arrays_cannot_hold_arrays() {
+    let mut t = BindingArrayFixture::new();
+    let _binding_array = t.module.types.insert(
+        naga::Type {
+            name: Some("binding_array_of_array".into()),
+            inner: naga::TypeInner::BindingArray {
+                base: t.ty_array,
+                size: naga::ArraySize::Dynamic,
+            },
+        },
+        t.span,
+    );
+
+    assert!(t.validator.validate(&t.module).is_err());
+}
+
+#[test]
+fn binding_arrays_cannot_hold_scalars() {
+    let mut t = BindingArrayFixture::new();
+    let _binding_array = t.module.types.insert(
+        naga::Type {
+            name: Some("binding_array_of_scalar".into()),
+            inner: naga::TypeInner::BindingArray {
+                base: t.ty_u32,
+                size: naga::ArraySize::Dynamic,
+            },
+        },
+        t.span,
+    );
+
+    assert!(t.validator.validate(&t.module).is_err());
+}