Validation rules for WGSL modules: binding collisions

[kvark]

We want to do shader translation at createShaderModule time, so we need to ensure that the WGSL module is valid to start with. So the first question is whether we are going to have the formal validation rules for WGSL modules.

The second question, derived from the first, is whether we allow binding collisions in this validation:

[[set = 0, binding = 1]] Texture1D<f32> MyTexture1;
[[set = 0, binding = 1]] Texture2D<u32> MyTexture2;
[[set = 0, binding = 1]] Texture3D<i32> MyTexture3;

Background: this question stems from #573 and the fact SPIR-V doesn't separate between comparison and regular samplers at the declaration level. If we want to be able to translate the whole SPIR-V module into WGSL, we'll need to allow binding collisions for WGSL module validation.

Alternatives considered: don't validate WGSL module as a whole, only validate specific entry points at pipeline creation time.

[dj2]

I don't think we want to permit binding collisions, that seems like it's just going to make things a lot harder to read/understand. Same with doing validation of entry points only.

I guess this would come back too, do we have multiple sampler types in WGSL or just one? Would that be work-able in MSL and HLSL? I'll have to go back and look and see how that would shake out.

I believe it was also mentioned that WebGPU separates the sampler types? So, would that be overly confusing to have WebGPU have multiple and WGSL have single?

[kvark]

Another option we have for the problem is considering the static use across the whole SPIR-V module, not an entry point. That may potentially treat some SPIR-V modules as invalid, but since this is all off-line, fixable, and unlikely to happen in practice, seems like the easiest to proceed with for now.

I don't think we want to permit binding collisions, that seems like it's just going to make things a lot harder to read/understand.

I agree, that would be asking for too much trouble, potentially :)

Another interesting question here is about depth textures. We agreed that WGSL separates depth from non-depth textures (in a sense of using the comparison samplers on them, not the texture format). But we don't have that on the WebGPU API binding side. Maybe we should consider having a separate binding type, like compared-texture or something (cc @Kangz ).

[kainino0x]

Does SPIR-V allow collisions in this way? I would expect it would - distinguished by the fact that SPIR-V makes it explicit which bindings are used by an entry point, and WGSL doesn't.

[kainino0x]

I believe it was also mentioned that WebGPU separates the sampler types?

Not at the type level, but yes - in the creation arguments (GPUSamplerDescriptor.compare) and bindings ("comparison-sampler").

[kvark]

There is also a possibility of defining all the affected bindings and inputs/outputs in the function/entry point itself:

fn vertex_main(
  [[location 0]] var a_particlePos : vec2<f32>,
  [[location 1]] var a_particleVel : vec2<f32>,
  [[set 1, binding 0]] var<uniform> params : SimParams,
  [[set 0, binding 1]] var<storage_buffer> particlesA : Particles,
) -> VertexOutput {
...
}

Doing this would MSL and partially HLSL, would make this issue more approachable, and also resolve #592 automatically.

[dj2]

Having them all in the entry point makes the scoping weird for non-entry point functions which use those variables.

[lachlansneff]

Could require the variables to be passed into any function that uses them?

[dneto0]

Does SPIR-V allow collisions in this way?

SPIR-V provides the mechanism, but Vulkan has the policy. Vulkan's policy is to look at static use within the call tree of the entry point being used. Furthermore, it allows a binding to be reused for different views of the same attached resource. E.g. you can have a storage buffer a texel buffer at the same descriptor set and binding. This expresses aliased views of the same locations. (And this shows up in the memory model too. To have writes down one variable show up in the view from the other variable, you need extra synchronization.)

But since WebGPU won't allow aliasing of a resource via separate variables, the relevant aspect is that Vulkan only checks static use in an entry point call tree.

[dneto0]

Could require the variables to be passed into any function that uses them?

That assumes a pretty general pointer or reference passing set of semantics. I think we've agreed to not go that direction.

[kvark]

@dneto0 hold on. If SPIR-V allows that and considers static use by a function to find out which one of conflicting bindings to use, the same policy can be done in WGSL. We also already have the static use by an entry point specified.
The benefit of this is that you'd be able to do SPIRV -> WGSL without putting extra requirements on the input (like that a sampler has to be a comparison sampler for the whole module as opposed to any individual entry point).

[dneto0]

@dneto0 hold on. If SPIR-V allows that and considers static use by a function to find out which one of conflicting bindings to use, the same policy can be done in WGSL. We also already have the static use by an entry point specified.

I agree. I think WGSL can limit itself to only looking for reuse (conflict) among static references within an entry point. Even if you have multiple entry points with lots of overlap among helper functions, you can still do it in one pass.

[grorg]

Was discussed at the 2020-08-04 meeting.

agreement - collisions in bindings are allowed in the module (but not in static use of an entry point)

[Kangz]

Bindings must not alias within a shader stage: two different variables in the resource interface of a given shader must not have the same group and binding values, when considered as a pair of values.

This restriction was added to the spec, so this issue can be closed.