Binding array containing a struct with a runtime array causes invalid spirv

[Vecvec]

Description When converting a wgsl shader with a binding array that contains a struct with a runtime array causes the spirv to have two OpDecorate Blocks. This is against the spirv specification https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#Decoration and a similar (but much longer) piece of code causes a validation error

VUID-VkShaderModuleCreateInfo-pCode-08737(ERROR / SPEC): msgNum: -1520283006 - Validation Error: [ VUID-VkShaderModuleCreateInfo-pCode-08737 ] | MessageID = 0xa5625282 | vkCreateShaderModule(): pCreateInfo->pCode (spirv-val produced an error):
ID '17' decorated with Block multiple times is not allowed.
  %MaterialsIdx = OpTypeStruct %_runtimearr_uint
. The Vulkan spec states: If pCode is a pointer to SPIR-V code, pCode must adhere to the validation rules described by the Validation Rules within a Module section of the SPIR-V Environment appendix (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkShaderModuleCreateInfo-pCode-08737)

Repro steps converting this shader to spirv causes invalid spirv to generate

struct Array {
    arr: array<u32>,
}

@group(0) @binding(0)
var<storage> store: binding_array<Array>;

Expected vs observed behavior Naga should not generate invalid spirv

Extra materials seems to be caused by https://github.com/gfx-rs/wgpu/blob/60a14c67fb43e68b7e64bee45ab3fac3f4253e72/naga/src/back/spv/writer.rs#L1045-L1047 and https://github.com/gfx-rs/wgpu/blob/60a14c67fb43e68b7e64bee45ab3fac3f4253e72/naga/src/back/spv/writer.rs#L1762-L1765

The invalid spirv generated

I'm not sure how this could be fixed, possibly with a hash-map of the id or a marker on the type id?

Platform trunk as of 60a14c67fb43e68b7e64bee45ab3fac3f4253e72

[teoxoy]

https://github.com/gfx-rs/wgpu/blob/60a14c67fb43e68b7e64bee45ab3fac3f4253e72/naga/src/back/spv/writer.rs#L1762-L1765

We shouldn't emit the decoration if base is a struct containing a runtime sized array.

[Vecvec]

Wouldn't it be better to handle the general case? Something like this (decorated_ids is a hashmap)

    pub(super) fn decorate(&mut self, id: Word, decoration: spirv::Decoration, operands: &[Word]) {
        match self.decorated_ids.get(&(id, decoration)) {
            None => {
                self.annotations
                    .push(Instruction::decorate(id, decoration, operands));
                self.decorated_ids.insert((id, decoration), ());
            }
            Some(_) => (),
        }
    }

could probably fix this too, and the function could be modified to warn/panic/error if there are operands and there is already a duplicate id.

[teoxoy]

In most cases we don't decorate things in multiple places with the same decoration. Runtime arrays are an edge case where SPIR-V's validation wants us to always decorate the struct even if it's unused.

[Vecvec]

So something closer to this

let ty = &ir_module.types[base];
let mut should_decorate = true;
// Check if the type has a runtime array.
// A normal runtime array gets validated out,
// so only structs can be with runtime arrays
if let crate::TypeInner::Struct { members, .. } = &ty.inner {
     // only the last member in a struct can be dynamically sized
     if let Some(last_member) = members.last() {
         if let crate::TypeInner::Array { size: ArraySize::Dynamic, .. } = &ir_module.types[last_member.ty].inner {
              should_decorate = false;
         }
     }
}
if should_decorate {
     let decorated_id = self.get_type_id(LookupType::Handle(base));
     self.decorate(decorated_id, Decoration::Block, &[]);
}

instead of https://github.com/gfx-rs/wgpu/blob/60a14c67fb43e68b7e64bee45ab3fac3f4253e72/naga/src/back/spv/writer.rs#L1763-L1764

[teoxoy]

Yeah, I think that would do it.