[spv-out] Always give structs with runtime arrays a Block decoration

[TheoDulka]

Fixes #2322, and so will fix bevyengine/bevy#8733 and fix webonnx/wonnx#161

I'm sure the code can be polished and expanded on to take BufferBlocks into account, but I just based it off of https://vulkan.lunarg.com/doc/view/1.3.246.1/windows/1.3-extensions/vkspec.html#interfaces-resources-descset, table 21. Where uniform buffers, storage buffers, and inline uniform blocks all use the Block decoration.

[teoxoy]

Looks good but I think we can now remove:

naga/src/back/spv/writer.rs

Lines 1657 to 1669 in 0491d39

	// 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.
	if let crate::AddressSpace::Storage { .. } = global_variable.space {
	let decorated_id = match ir_module.types[global_variable.ty].inner {
	crate::TypeInner::BindingArray { base, .. } => {
	self.get_type_id(LookupType::Handle(base))
	}
	_ => inner_type_id,
	};
	self.decorate(decorated_id, Decoration::Block, &[]);
	}

Although:

Do the validation layers not require that the struct is used by a variable in the storage address space despite it seemingly being required by the VUID?

[TheoDulka]

Removing the entire block decorating chunk would cause a problem, because this

naga/tests/out/spv/binding-buffer-arrays.spvasm

Line 19 in 0491d39

OpDecorate %5 Block

gets removed, leading to

error: line 35: Target of NonWritable decoration is invalid: must point to a storage image, uniform block, or storage buffer
  %8 = OpVariable %_ptr_StorageBuffer__arr__struct_6_uint_10 StorageBuffer

as an error.

Instead, I removed the extra decorating of the non-BindingArrays, which seems to work.

---

For your other comment, from what I can tell (minus binding-buffer-arrays.wgsl) the Block decorated structs are also in a StorageBuffer. An example in boids:

boids.spvasm

OpName %9 "Particles"
...
OpDecorate %9 Block
...
%17 = OpTypePointer StorageBuffer %9

from

naga/tests/in/boids.wgsl

Lines 18 to 20 in 0491d39

	struct Particles {
	particles : array<Particle>
	}

[teoxoy]

Removing the entire block decorating chunk would cause a problem, because this

naga/tests/out/spv/binding-buffer-arrays.spvasm

Line 19 in 0491d39

OpDecorate %5 Block

gets removed, leading to

error: line 35: Target of NonWritable decoration is invalid: must point to a storage image, uniform block, or storage buffer
  %8 = OpVariable %_ptr_StorageBuffer__arr__struct_6_uint_10 StorageBuffer

as an error.

Instead, I removed the extra decorating of the non-BindingArrays, which seems to work.

I see, that makes sense; the comment was out of date. Can you update it or remove it?

For your other comment, from what I can tell (minus binding-buffer-arrays.wgsl) the Block decorated structs are also in a StorageBuffer. An example in boids:

boids.spvasm

OpName %9 "Particles"
...
OpDecorate %9 Block
...
%17 = OpTypePointer StorageBuffer %9

from

naga/tests/in/boids.wgsl

Lines 18 to 20 in 0491d39

	struct Particles {
	particles : array<Particle>
	}

Right but what I meant is if the struct doesn't get used at all; is it fine to always decorate them with Block even if unused?

In your example in #2322, the VUID gets triggered because we are missing the Block decoration but it might still get triggered even with the Block decoration since it's unused by a global variable in the StorageBuffer address space; at least that's how I'm interpreting the VUID.

[teoxoy]

Wait, I don't understand why the VUID gets triggered, the validation code for this VUID is inside ValidateVariable

https://github.com/KhronosGroup/SPIRV-Tools/blob/9b923f7cc3dde6e1a4886b577677e52c3093ffcc/source/val/validate_memory.cpp#L758-L762

It only checks global variables. And we already decorate structs that have a runtime array if they are used by a global variable.

Can you share a WGSL shader that when ran through naga's SPV backend fails validation with the VUID? Or even better, add it as a snapshot test to the PR.

[TheoDulka]

Oh I didn't make it clear, but initially I followed your recommendation and removed the entire chunk of code you thought could be removed. When I did, it caused binding-buffer-arrays.wgsl to give the error I showed when using xtest, because it required the binding array in the StorageBuffer to be Block decorated (I think), which is not addressed by 58aa529.

So instead, in the new commit I pushed a755828, I modified that chunk of code to still Block decorate BindingArrays in storage spaces, but no longer decorate anything else which produced multiple Block decorations. This seems to work just fine.

Should I still make another wgsl shader in snapshot if binding-buffer-arrays.wgsl was enough to give the error?

[teoxoy]

My main question is why does the current code in master trigger the validation error at all?

The current code already writes the Block decoration on a struct containing the runtime array if used by a global variable.

Maybe I'm missing something and wanted to reproduce it locally.

[TheoDulka]

Ahh I misunderstood, I see what you mean now. Yeah it does seem to only trigger when it's not used by a global variable. The bevy issue is from this bevy file mesh_view_types.wgsl, where the file doesn't use PointLights in a global variable, as it's used for importing the types. I assume it's the same case with the wonnx issue, ArrayVector might not be used. So I guess the validation checks if the struct has a Block decoration regardless of use in variables? The reason is beyond me.

I didn't realize my code just ended up moving the Block decorations, as the runtime arrays are already accounted for, but only when they're used in a global variable. When they aren't, that's when the validation error shows up. At this point my code handles runtime arrays in structs regardless of if the struct is used.

---

I added runtime-array-in-unused-struct.wgsl to the snapshots and changed the comment of the storage binding array decorating.

[TheoDulka]

@teoxoy I found out why it doesn't matter if it's in a variable, because the error from the issues is spit out when checking the types, in ValidateTypeStruct

https://github.com/KhronosGroup/SPIRV-Tools/blob/9b923f7cc3dde6e1a4886b577677e52c3093ffcc/source/val/validate_type.cpp#L353-L360

The link you sent was for a different validation error when checking variables

Wait, I don't understand why the VUID gets triggered, the validation code for this VUID is inside ValidateVariable

https://github.com/KhronosGroup/SPIRV-Tools/blob/9b923f7cc3dde6e1a4886b577677e52c3093ffcc/source/val/validate_memory.cpp#L758-L762

[teoxoy]

Ah, I see! Thank you for tracking it down!

I think we can go ahead with this PR and omit unused structs in the future if it becomes an issue that we are decorating them despite them being unused.

Related:
KhronosGroup/SPIRV-Tools#5094
KhronosGroup/glslang#2439
KhronosGroup/glslang#3130