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

TheoDulka · 2023-08-25T05:07:28Z

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.

…untimeArray must be decorated with Block or BufferBlock."

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?

…Array`, removes the extra block decorations

TheoDulka · 2023-09-01T00:23:53Z

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 · 2023-09-01T09:45:48Z

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 · 2023-09-01T10:05:18Z

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 · 2023-09-01T13:03:19Z

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 · 2023-09-01T14:05:07Z

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 · 2023-09-01T19:16:20Z

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 · 2023-09-03T16:59:08Z

@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 · 2023-09-12T17:57:55Z

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

[spv-out] Fixed validation error: "OpTypeStruct containing an OpTypeR…

58aa529

…untimeArray must be decorated with Block or BufferBlock."

teoxoy requested changes Aug 31, 2023

View reviewed changes

Modified the previous block decorating to only decorate with `Binding…

a755828

…Array`, removes the extra block decorations

TheoDulka requested a review from teoxoy September 1, 2023 00:24

Added to the convert_wgsl test, with runtime-array-in-unused-struct

adbb806

teoxoy approved these changes Sep 12, 2023

View reviewed changes

teoxoy changed the title ~~[spv-out] Give structs with runtime arrays a Block decoration~~ [spv-out] Always give structs with runtime arrays a Block decoration Sep 12, 2023

teoxoy merged commit 1281c11 into gfx-rs:master Sep 12, 2023
6 checks passed

chenrui333 mentioned this pull request Oct 25, 2023

naga-cli 0.14.0 Homebrew/homebrew-core#152392

Merged

djeedai mentioned this pull request Dec 15, 2023

Error occurred during runtime bevyengine/bevy#10987

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TheoDulka commented Aug 25, 2023 •

edited

Loading

teoxoy left a comment

TheoDulka commented Sep 1, 2023

teoxoy commented Sep 1, 2023

teoxoy commented Sep 1, 2023

TheoDulka commented Sep 1, 2023

teoxoy commented Sep 1, 2023

TheoDulka commented Sep 1, 2023

TheoDulka commented Sep 3, 2023

teoxoy commented Sep 12, 2023

	// 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, &[]);
	}

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

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

Conversation

TheoDulka commented Aug 25, 2023 • edited Loading

teoxoy left a comment

Choose a reason for hiding this comment

TheoDulka commented Sep 1, 2023

teoxoy commented Sep 1, 2023

teoxoy commented Sep 1, 2023

TheoDulka commented Sep 1, 2023

teoxoy commented Sep 1, 2023

TheoDulka commented Sep 1, 2023

TheoDulka commented Sep 3, 2023

teoxoy commented Sep 12, 2023

TheoDulka commented Aug 25, 2023 •

edited

Loading