[msl] fix vertex pulling with a stride of 0 (#8265)

Author: teoxoy

cts_runner/test.lst

@@ -18,7 +18,7 @@ webgpu:api,operation,compute,basic:memcpy:*
 webgpu:api,operation,compute_pipeline,overrides:*
 webgpu:api,operation,device,lost:*
 webgpu:api,operation,render_pass,storeOp:*
-fails-if(metal,vulkan) webgpu:api,operation,vertex_state,correctness:array_stride_zero:*
+fails-if(vulkan) webgpu:api,operation,vertex_state,correctness:array_stride_zero:*
 // Presumably vertex pulling, revisit after https://github.com/gfx-rs/wgpu/issues/7981 is fixed.
 fails-if(metal) webgpu:api,operation,vertex_state,correctness:setVertexBuffer_offset_and_attribute_offset:*
 fails-if(dx12) webgpu:api,validation,capability_checks,limits,maxBindGroups:setBindGroup,*

naga/src/back/msl/mod.rs

@@ -418,6 +418,17 @@ pub enum VertexFormat {
     Unorm8x4Bgra = 44,
 }
 
+/// Defines how to advance the data in vertex buffers.
+#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "serialize", derive(serde::Serialize))]
+#[cfg_attr(feature = "deserialize", derive(serde::Deserialize))]
+pub enum VertexBufferStepMode {
+    Constant,
+    #[default]
+    ByVertex,
+    ByInstance,
+}
+
 /// A mapping of vertex buffers and their attributes to shader
 /// locations.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -446,9 +457,8 @@ pub struct VertexBufferMapping {
     pub id: u32,
     /// Size of the structure in bytes
     pub stride: u32,
-    /// True if the buffer is indexed by vertex, false if indexed
-    /// by instance.
-    pub indexed_by_vertex: bool,
+    /// Vertex buffer step mode
+    pub step_mode: VertexBufferStepMode,
     /// Vec of the attributes within the structure
     pub attributes: Vec<AttributeMapping>,
 }

naga/src/back/msl/writer.rs

@@ -6359,7 +6359,7 @@ template <typename A>
         struct VertexBufferMappingResolved<'a> {
             id: u32,
             stride: u32,
-            indexed_by_vertex: bool,
+            step_mode: back::msl::VertexBufferStepMode,
             ty_name: String,
             param_name: String,
             elem_name: String,
@@ -6395,10 +6395,14 @@ template <typename A>
                     "Vertex pulling requires a non-zero buffer stride."
                 );
 
-                if vbm.indexed_by_vertex {
-                    needs_vertex_id = true;
-                } else {
-                    needs_instance_id = true;
+                match vbm.step_mode {
+                    back::msl::VertexBufferStepMode::Constant => {}
+                    back::msl::VertexBufferStepMode::ByVertex => {
+                        needs_vertex_id = true;
+                    }
+                    back::msl::VertexBufferStepMode::ByInstance => {
+                        needs_instance_id = true;
+                    }
                 }
 
                 let buffer_ty = self.namer.call(format!("vb_{buffer_id}_type").as_str());
@@ -6408,7 +6412,7 @@ template <typename A>
                 vbm_resolved.push(VertexBufferMappingResolved {
                     id: buffer_id,
                     stride: buffer_stride,
-                    indexed_by_vertex: vbm.indexed_by_vertex,
+                    step_mode: vbm.step_mode,
                     ty_name: buffer_ty,
                     param_name: buffer_param,
                     elem_name: buffer_elem,
@@ -7199,8 +7203,6 @@ template <typename A>
             }
 
             if do_vertex_pulling {
-                assert!(needs_vertex_id || needs_instance_id);
-
                 let mut separator = if is_first_argument {
                     is_first_argument = false;
                     ' '
@@ -7278,16 +7280,22 @@ template <typename A>
 
                     let idx = &vbm.id;
                     let stride = &vbm.stride;
-                    let index_name = if vbm.indexed_by_vertex {
-                        if let Some(ref name) = v_existing_id {
-                            name
-                        } else {
-                            &v_id
+                    let index_name = match vbm.step_mode {
+                        back::msl::VertexBufferStepMode::Constant => "0",
+                        back::msl::VertexBufferStepMode::ByVertex => {
+                            if let Some(ref name) = v_existing_id {
+                                name
+                            } else {
+                                &v_id
+                            }
+                        }
+                        back::msl::VertexBufferStepMode::ByInstance => {
+                            if let Some(ref name) = i_existing_id {
+                                name
+                            } else {
+                                &i_id
+                            }
                         }
-                    } else if let Some(ref name) = i_existing_id {
-                        name
-                    } else {
-                        &i_id
                     };
                     write!(
                         self.out,

naga/tests/in/wgsl/msl-vpt-formats-x1.toml

@@ -49,5 +49,5 @@ attributes = [
     { offset = 640, shader_location = 40, format = "unorm8x4-bgra" },
 ]
 id = 1
-indexed_by_vertex = true
+step_mode = "ByVertex"
 stride = 644

naga/tests/in/wgsl/msl-vpt-formats-x2.toml

@@ -49,5 +49,5 @@ attributes = [
     { offset = 640, shader_location = 40, format = "unorm8x4-bgra" },
 ]
 id = 1
-indexed_by_vertex = true
+step_mode = "ByVertex"
 stride = 644

naga/tests/in/wgsl/msl-vpt-formats-x3.toml

@@ -49,5 +49,5 @@ attributes = [
     { offset = 640, shader_location = 40, format = "unorm8x4-bgra" },
 ]
 id = 1
-indexed_by_vertex = true
+step_mode = "ByVertex"
 stride = 644

naga/tests/in/wgsl/msl-vpt-formats-x4.toml

@@ -49,5 +49,5 @@ attributes = [
     { offset = 640, shader_location = 40, format = "unorm8x4-bgra" },
 ]
 id = 1
-indexed_by_vertex = true
+step_mode = "ByVertex"
 stride = 644

naga/tests/in/wgsl/msl-vpt.toml

@@ -10,11 +10,11 @@ attributes = [
     { offset = 4, shader_location = 1, format = "Float32x4" },
 ]
 id = 1
-indexed_by_vertex = true
+step_mode = "ByVertex"
 stride = 20
 
 [[msl_pipeline.vertex_buffer_mappings]]
 attributes = [{ offset = 0, shader_location = 2, format = "Float32x2" }]
 id = 2
-indexed_by_vertex = false
+step_mode = "ByInstance"
 stride = 16

tests/tests/wgpu-gpu/vertex_state.rs

@@ -3,7 +3,7 @@ use wgpu::{
     vertex_attr_array,
 };
 use wgpu_test::{
-    gpu_test, FailureCase, GpuTestConfiguration, GpuTestInitializer, TestParameters, TestingContext,
+    gpu_test, GpuTestConfiguration, GpuTestInitializer, TestParameters, TestingContext,
 };
 
 pub fn all_tests(vec: &mut Vec<GpuTestInitializer>) {
@@ -12,11 +12,7 @@ pub fn all_tests(vec: &mut Vec<GpuTestInitializer>) {
 
 #[gpu_test]
 static SET_ARRAY_STRIDE_TO_0: GpuTestConfiguration = GpuTestConfiguration::new()
-    .parameters(
-        TestParameters::default()
-            .limits(wgpu::Limits::downlevel_defaults())
-            .expect_fail(FailureCase::backend(wgpu::Backends::METAL)),
-    )
+    .parameters(TestParameters::default().limits(wgpu::Limits::downlevel_defaults()))
     .run_async(set_array_stride_to_0);
 
 /// Tests that draws using a vertex buffer with stride of 0 works correctly (especially on the

wgpu-hal/src/metal/device.rs

@@ -1154,7 +1154,15 @@ impl crate::Device for super::Device {
                                 .try_into()
                                 .unwrap()
                         },
-                        indexed_by_vertex: (vbl.step_mode == wgt::VertexStepMode::Vertex {}),
+                        step_mode: match (vbl.array_stride == 0, vbl.step_mode) {
+                            (true, _) => naga::back::msl::VertexBufferStepMode::Constant,
+                            (false, wgt::VertexStepMode::Vertex) => {
+                                naga::back::msl::VertexBufferStepMode::ByVertex
+                            }
+                            (false, wgt::VertexStepMode::Instance) => {
+                                naga::back::msl::VertexBufferStepMode::ByInstance
+                            }
+                        },
                         attributes,
                     });
                 }