From 58aa5296864043e6a7a9ad5a37b320dddfb59b18 Mon Sep 17 00:00:00 2001 From: TheoDulka Date: Fri, 25 Aug 2023 15:12:26 -0500 Subject: [PATCH 1/3] [spv-out] Fixed validation error: "OpTypeStruct containing an OpTypeRuntimeArray must be decorated with Block or BufferBlock." --- src/back/spv/writer.rs | 15 +++++++++++++++ tests/out/spv/access.spvasm | 1 + tests/out/spv/boids.spvasm | 1 + tests/out/spv/bounds-check-restrict.spvasm | 1 + tests/out/spv/bounds-check-zero.spvasm | 1 + tests/out/spv/collatz.spvasm | 1 + tests/out/spv/debug-symbol-terrain.spvasm | 2 ++ tests/out/spv/pointers.spvasm | 1 + 8 files changed, 23 insertions(+) diff --git a/src/back/spv/writer.rs b/src/back/spv/writer.rs index e6db93602a..0da113f865 100644 --- a/src/back/spv/writer.rs +++ b/src/back/spv/writer.rs @@ -1016,12 +1016,27 @@ impl Writer { ref members, span: _, } => { + let mut has_runtime_array = false; let mut member_ids = Vec::with_capacity(members.len()); for (index, member) in members.iter().enumerate() { + let member_ty = &arena[member.ty]; + match member_ty.inner { + crate::TypeInner::Array { + base: _, + size: crate::ArraySize::Dynamic, + stride: _, + } => { + has_runtime_array = true; + } + _ => (), + } self.decorate_struct_member(id, index, member, arena)?; let member_id = self.get_type_id(LookupType::Handle(member.ty)); member_ids.push(member_id); } + if has_runtime_array { + self.decorate(id, Decoration::Block, &[]); + } Instruction::type_struct(id, member_ids.as_slice()) } diff --git a/tests/out/spv/access.spvasm b/tests/out/spv/access.spvasm index f7be2338c5..65864ca677 100644 --- a/tests/out/spv/access.spvasm +++ b/tests/out/spv/access.spvasm @@ -73,6 +73,7 @@ OpMemberDecorate %20 2 Offset 96 OpMemberDecorate %20 3 Offset 100 OpMemberDecorate %20 4 Offset 144 OpMemberDecorate %20 5 Offset 160 +OpDecorate %20 Block OpMemberDecorate %22 0 Offset 0 OpMemberDecorate %22 0 ColMajor OpMemberDecorate %22 0 MatrixStride 8 diff --git a/tests/out/spv/boids.spvasm b/tests/out/spv/boids.spvasm index 94ee9e8d93..be6bc6c414 100644 --- a/tests/out/spv/boids.spvasm +++ b/tests/out/spv/boids.spvasm @@ -48,6 +48,7 @@ OpMemberDecorate %7 5 Offset 20 OpMemberDecorate %7 6 Offset 24 OpDecorate %8 ArrayStride 16 OpMemberDecorate %9 0 Offset 0 +OpDecorate %9 Block OpDecorate %13 DescriptorSet 0 OpDecorate %13 Binding 0 OpDecorate %14 Block diff --git a/tests/out/spv/bounds-check-restrict.spvasm b/tests/out/spv/bounds-check-restrict.spvasm index 8b13b44801..23c6f4bb39 100644 --- a/tests/out/spv/bounds-check-restrict.spvasm +++ b/tests/out/spv/bounds-check-restrict.spvasm @@ -15,6 +15,7 @@ OpMemberDecorate %10 2 Offset 64 OpMemberDecorate %10 2 ColMajor OpMemberDecorate %10 2 MatrixStride 16 OpMemberDecorate %10 3 Offset 112 +OpDecorate %10 Block OpDecorate %13 DescriptorSet 0 OpDecorate %13 Binding 0 OpDecorate %10 Block diff --git a/tests/out/spv/bounds-check-zero.spvasm b/tests/out/spv/bounds-check-zero.spvasm index b3c108d982..619ccedc72 100644 --- a/tests/out/spv/bounds-check-zero.spvasm +++ b/tests/out/spv/bounds-check-zero.spvasm @@ -15,6 +15,7 @@ OpMemberDecorate %10 2 Offset 64 OpMemberDecorate %10 2 ColMajor OpMemberDecorate %10 2 MatrixStride 16 OpMemberDecorate %10 3 Offset 112 +OpDecorate %10 Block OpDecorate %13 DescriptorSet 0 OpDecorate %13 Binding 0 OpDecorate %10 Block diff --git a/tests/out/spv/collatz.spvasm b/tests/out/spv/collatz.spvasm index f8212185ca..aabed55023 100644 --- a/tests/out/spv/collatz.spvasm +++ b/tests/out/spv/collatz.spvasm @@ -19,6 +19,7 @@ OpName %48 "global_id" OpName %51 "main" OpDecorate %4 ArrayStride 4 OpMemberDecorate %5 0 Offset 0 +OpDecorate %5 Block OpDecorate %7 DescriptorSet 0 OpDecorate %7 Binding 0 OpDecorate %5 Block diff --git a/tests/out/spv/debug-symbol-terrain.spvasm b/tests/out/spv/debug-symbol-terrain.spvasm index fcc14e4852..a76ae60497 100644 --- a/tests/out/spv/debug-symbol-terrain.spvasm +++ b/tests/out/spv/debug-symbol-terrain.spvasm @@ -415,8 +415,10 @@ OpMemberDecorate %14 0 Offset 0 OpMemberDecorate %14 1 Offset 16 OpDecorate %15 ArrayStride 32 OpMemberDecorate %16 0 Offset 0 +OpDecorate %16 Block OpDecorate %17 ArrayStride 4 OpMemberDecorate %18 0 Offset 0 +OpDecorate %18 Block OpMemberDecorate %20 0 Offset 0 OpMemberDecorate %20 1 Offset 8 OpMemberDecorate %20 2 Offset 16 diff --git a/tests/out/spv/pointers.spvasm b/tests/out/spv/pointers.spvasm index 3debe27b16..0f1e7a7d0a 100644 --- a/tests/out/spv/pointers.spvasm +++ b/tests/out/spv/pointers.spvasm @@ -20,6 +20,7 @@ OpName %35 "v" OpName %36 "index_dynamic_array" OpDecorate %6 ArrayStride 4 OpMemberDecorate %7 0 Offset 0 +OpDecorate %7 Block OpDecorate %10 DescriptorSet 0 OpDecorate %10 Binding 0 OpDecorate %7 Block From a7558285c98f044ed1933ea80307e732c8e2e438 Mon Sep 17 00:00:00 2001 From: TheoDulka Date: Thu, 31 Aug 2023 19:11:33 -0500 Subject: [PATCH 2/3] Modified the previous block decorating to only decorate with `BindingArray`, removes the extra block decorations --- src/back/spv/writer.rs | 8 ++++---- tests/out/spv/access.spvasm | 1 - tests/out/spv/boids.spvasm | 2 -- tests/out/spv/bounds-check-restrict.spvasm | 1 - tests/out/spv/bounds-check-zero.spvasm | 1 - tests/out/spv/collatz.spvasm | 1 - tests/out/spv/debug-symbol-terrain.spvasm | 2 -- tests/out/spv/pointers.spvasm | 1 - 8 files changed, 4 insertions(+), 13 deletions(-) diff --git a/src/back/spv/writer.rs b/src/back/spv/writer.rs index 0da113f865..d0be0a07dd 100644 --- a/src/back/spv/writer.rs +++ b/src/back/spv/writer.rs @@ -1670,13 +1670,13 @@ impl Writer { // a runtime-sized array. In this case, we need to decorate it with // Block. if let crate::AddressSpace::Storage { .. } = global_variable.space { - let decorated_id = match ir_module.types[global_variable.ty].inner { + match ir_module.types[global_variable.ty].inner { crate::TypeInner::BindingArray { base, .. } => { - self.get_type_id(LookupType::Handle(base)) + let decorated_id = self.get_type_id(LookupType::Handle(base)); + self.decorate(decorated_id, Decoration::Block, &[]); } - _ => inner_type_id, + _ => (), }; - self.decorate(decorated_id, Decoration::Block, &[]); } if substitute_inner_type_lookup.is_some() { inner_type_id diff --git a/tests/out/spv/access.spvasm b/tests/out/spv/access.spvasm index 65864ca677..cb0cdeda27 100644 --- a/tests/out/spv/access.spvasm +++ b/tests/out/spv/access.spvasm @@ -87,7 +87,6 @@ OpDecorate %33 ArrayStride 4 OpDecorate %36 ArrayStride 16 OpDecorate %47 DescriptorSet 0 OpDecorate %47 Binding 0 -OpDecorate %20 Block OpDecorate %49 DescriptorSet 0 OpDecorate %49 Binding 1 OpDecorate %50 Block diff --git a/tests/out/spv/boids.spvasm b/tests/out/spv/boids.spvasm index be6bc6c414..f57d997238 100644 --- a/tests/out/spv/boids.spvasm +++ b/tests/out/spv/boids.spvasm @@ -56,10 +56,8 @@ OpMemberDecorate %14 0 Offset 0 OpDecorate %16 NonWritable OpDecorate %16 DescriptorSet 0 OpDecorate %16 Binding 1 -OpDecorate %9 Block OpDecorate %18 DescriptorSet 0 OpDecorate %18 Binding 2 -OpDecorate %9 Block OpDecorate %36 BuiltIn GlobalInvocationId %2 = OpTypeVoid %3 = OpTypeInt 32 0 diff --git a/tests/out/spv/bounds-check-restrict.spvasm b/tests/out/spv/bounds-check-restrict.spvasm index 23c6f4bb39..69e612022f 100644 --- a/tests/out/spv/bounds-check-restrict.spvasm +++ b/tests/out/spv/bounds-check-restrict.spvasm @@ -18,7 +18,6 @@ OpMemberDecorate %10 3 Offset 112 OpDecorate %10 Block OpDecorate %13 DescriptorSet 0 OpDecorate %13 Binding 0 -OpDecorate %10 Block %2 = OpTypeVoid %3 = OpTypeFloat 32 %6 = OpTypeInt 32 0 diff --git a/tests/out/spv/bounds-check-zero.spvasm b/tests/out/spv/bounds-check-zero.spvasm index 619ccedc72..f24ab2cc73 100644 --- a/tests/out/spv/bounds-check-zero.spvasm +++ b/tests/out/spv/bounds-check-zero.spvasm @@ -18,7 +18,6 @@ OpMemberDecorate %10 3 Offset 112 OpDecorate %10 Block OpDecorate %13 DescriptorSet 0 OpDecorate %13 Binding 0 -OpDecorate %10 Block %2 = OpTypeVoid %3 = OpTypeFloat 32 %6 = OpTypeInt 32 0 diff --git a/tests/out/spv/collatz.spvasm b/tests/out/spv/collatz.spvasm index aabed55023..310bc5c1ef 100644 --- a/tests/out/spv/collatz.spvasm +++ b/tests/out/spv/collatz.spvasm @@ -22,7 +22,6 @@ OpMemberDecorate %5 0 Offset 0 OpDecorate %5 Block OpDecorate %7 DescriptorSet 0 OpDecorate %7 Binding 0 -OpDecorate %5 Block OpDecorate %48 BuiltIn GlobalInvocationId %2 = OpTypeVoid %3 = OpTypeInt 32 0 diff --git a/tests/out/spv/debug-symbol-terrain.spvasm b/tests/out/spv/debug-symbol-terrain.spvasm index a76ae60497..ac83cc1239 100644 --- a/tests/out/spv/debug-symbol-terrain.spvasm +++ b/tests/out/spv/debug-symbol-terrain.spvasm @@ -444,10 +444,8 @@ OpDecorate %30 Block OpMemberDecorate %30 0 Offset 0 OpDecorate %32 DescriptorSet 0 OpDecorate %32 Binding 1 -OpDecorate %16 Block OpDecorate %34 DescriptorSet 0 OpDecorate %34 Binding 2 -OpDecorate %18 Block OpDecorate %36 DescriptorSet 0 OpDecorate %36 Binding 0 OpDecorate %37 Block diff --git a/tests/out/spv/pointers.spvasm b/tests/out/spv/pointers.spvasm index 0f1e7a7d0a..332d265fad 100644 --- a/tests/out/spv/pointers.spvasm +++ b/tests/out/spv/pointers.spvasm @@ -23,7 +23,6 @@ OpMemberDecorate %7 0 Offset 0 OpDecorate %7 Block OpDecorate %10 DescriptorSet 0 OpDecorate %10 Binding 0 -OpDecorate %7 Block %2 = OpTypeVoid %4 = OpTypeInt 32 1 %3 = OpTypeVector %4 2 From adbb8063617c15b41dd585def84852c132920717 Mon Sep 17 00:00:00 2001 From: TheoDulka Date: Fri, 1 Sep 2023 14:09:13 -0500 Subject: [PATCH 3/3] Added to the `convert_wgsl` test, with `runtime-array-in-unused-struct` --- src/back/spv/writer.rs | 5 ++-- tests/in/runtime-array-in-unused-struct.wgsl | 12 ++++++++++ .../spv/runtime-array-in-unused-struct.spvasm | 24 +++++++++++++++++++ tests/snapshots.rs | 1 + 4 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 tests/in/runtime-array-in-unused-struct.wgsl create mode 100644 tests/out/spv/runtime-array-in-unused-struct.spvasm diff --git a/src/back/spv/writer.rs b/src/back/spv/writer.rs index d0be0a07dd..da5048c0c3 100644 --- a/src/back/spv/writer.rs +++ b/src/back/spv/writer.rs @@ -1667,8 +1667,9 @@ impl Writer { } else { // This is a global variable in the Storage address space. The only // way it could have `global_needs_wrapper() == false` is if it has - // a runtime-sized array. In this case, we need to decorate it with - // Block. + // a runtime-sized or binding array. + // Runtime-sized arrays were decorated when iterating through struct content. + // Now binding arrays require Block decorating. if let crate::AddressSpace::Storage { .. } = global_variable.space { match ir_module.types[global_variable.ty].inner { crate::TypeInner::BindingArray { base, .. } => { diff --git a/tests/in/runtime-array-in-unused-struct.wgsl b/tests/in/runtime-array-in-unused-struct.wgsl new file mode 100644 index 0000000000..bcee56d9b0 --- /dev/null +++ b/tests/in/runtime-array-in-unused-struct.wgsl @@ -0,0 +1,12 @@ +struct DataStruct { + data: f32, + data_vec: vec4, +} + +struct Struct { + data: array, +}; + +struct PrimitiveStruct { + data: array, +}; diff --git a/tests/out/spv/runtime-array-in-unused-struct.spvasm b/tests/out/spv/runtime-array-in-unused-struct.spvasm new file mode 100644 index 0000000000..ab3bd3d01b --- /dev/null +++ b/tests/out/spv/runtime-array-in-unused-struct.spvasm @@ -0,0 +1,24 @@ +; SPIR-V +; Version: 1.1 +; Generator: rspirv +; Bound: 10 +OpCapability Shader +OpCapability Linkage +%1 = OpExtInstImport "GLSL.std.450" +OpMemoryModel Logical GLSL450 +OpMemberDecorate %5 0 Offset 0 +OpMemberDecorate %5 1 Offset 16 +OpDecorate %6 ArrayStride 32 +OpMemberDecorate %7 0 Offset 0 +OpDecorate %7 Block +OpDecorate %8 ArrayStride 4 +OpMemberDecorate %9 0 Offset 0 +OpDecorate %9 Block +%2 = OpTypeVoid +%3 = OpTypeFloat 32 +%4 = OpTypeVector %3 4 +%5 = OpTypeStruct %3 %4 +%6 = OpTypeRuntimeArray %5 +%7 = OpTypeStruct %6 +%8 = OpTypeRuntimeArray %3 +%9 = OpTypeStruct %8 \ No newline at end of file diff --git a/tests/snapshots.rs b/tests/snapshots.rs index e1e1144139..6c6075b2cf 100644 --- a/tests/snapshots.rs +++ b/tests/snapshots.rs @@ -602,6 +602,7 @@ fn convert_wgsl() { "workgroup-uniform-load", Targets::WGSL | Targets::GLSL | Targets::SPIRV | Targets::HLSL | Targets::METAL, ), + ("runtime-array-in-unused-struct", Targets::SPIRV), ("sprite", Targets::SPIRV), ("force_point_size_vertex_shader_webgl", Targets::GLSL), ("invariant", Targets::GLSL),