[spv-in] Support texture access qualifier and infer texture access mode

[schell]

Description
As I wrote about here and as far as I can tell, it's not possible to write to textures in wgpu if your shader is SPIR-V.

This seems to be because the OpTypeImage passed to OpImageWrite must have its "Sampled" parameter set to 2:

Image must be an object whose type is OpTypeImage with a Sampled operand of 0 or 2.

...

2 indicates an image compatible with read/write operations (a storage or subpass data image)

I think naga's spv frontend erroneously assumes that 2 in this case means read AND write instead of "one of readonly, writeonly or readwrite", as is implied by only having one value for all of them in the SPIR-V spec.

• related matrix discussion.

Repro steps
Working on it...

Expected vs observed behavior
I expect to be able to build my shader pipeline and write to storage textures.

Instead, I got this error:

wgpu error: Validation Error

Caused by:
  In Device::create_compute_pipeline, label = 'compute-occlusion-copy-depth-to-pyramid'
    Error matching shader requirements against the pipeline
      Shader global ResourceBinding { group: 0, binding: 2 } is not available in the pipeline layout
        Texture class Storage { format: R32Float, access: StorageAccess(STORE) } doesn't match the shader Storage { format: R32Float, access: StorageAccess(LOAD | STORE) }

I should note that simply marking my texture as read+write is not an option as that is a native-only feature and I target web.

[schell]

@jimblandy had mentioned later in the matrix channel that in order to properly support this, we may need to do something similar to the atomics upgrade.

What that means is storing the pointer to the image during parsing and then doing a late-pass over the generated module to inspect how that image is used (read or write or readwrite), and then updating the type accordingly.

[teoxoy]

OpTypeImage does optionally contain an Access Qualifier, as a start we can probably start consulting it and return an unimplemented error otherwise.

[schell]

tl;dr - I don't know if AccessQualifier can solve the problem.

@teoxoy I previously thought it might be the case that rust-gpu (which I use to generate my SPIR-V shaders) needed to specify the AccessQualifier so that wgpu knew how the image was going to be used. But after cutting a PR into rust-gpu to be able to set the AccessQualifier I found that using AccessQualifier won't pass spir-val as it requires the Kernel capability:

 error: Operand 9 of TypeImage requires one of these capabilities: Kernel 
           %16 = OpTypeImage %float 2D 2 0 0 2 Rgba8 WriteOnly

...and that capability isn't allowed in Vulkan

error: Capability Kernel is not allowed by Vulkan 1.1 specification (or requires extension)
           OpCapability Kernel

I'm fairly certain that AccessQualifier is only for OpenCL, @eddyb has a useful comment on it here, but it's hard to read the spec and determine what contexts Kernel is useful in.

It is a confusing part of the spec.

[eddyb]

I think naga's spv frontend erroneously assumes that 2 in this case means read AND write instead of "one of readonly, writeonly or readwrite", as is implied by only having one value for all of them in the SPIR-V spec.

As I mentioned in EmbarkStudios/rust-gpu#1097 (comment) (which @schell linked), NonReadable is how Vulkan SPIR-V expresses write-only (and NonWritable for read-only, presumably).

If Naga supports e.g. NonWritable storage buffers (which I'm pretty sure it does because I vaguely remember needing to emit that), this should be very similar.

---

In general, the biggest "life hack" for figuring out how Vulkan SPIR-V handles something is to ignore it and just go to GLSL (esp. for features that existed in OpenGL):
https://www.khronos.org/opengl/wiki/Type_Qualifier_(GLSL)#Memory_qualifiers

It is conceptually in the wrong place, but NonReadable is literally the GLSL writeonly and it's in the place GLSL implies (the OpVariable decorated with DescriptorSet+Binding, whether it's one image, an array of images, etc.).

[schell]

Thanks @eddyb :)

If I'm not mistaken, OpVariable will be defined after OpTypeImage, which puts us back into "type upgrade" territory.

[eddyb]

That reminds me of my preferred suggestion for the Rust-GPU side:

Image!(..., non_readable) - in the type, even if Vulkan SPIR-V does not store that information there

So in essence, both Naga and Rust-GPU's spirv_std::Image want it to be in the type (i.e. as if Image is a &[[Texel]] or &[[Atomic<Texel>]] etc. - and where the Image itself is stored, not really mattering), and have its operations checked based on the type alone.

And OpenCL ("Kernel") SPIR-V does have the right encoding, but Vulkan ("Shader") SPIR-V doesn't.

---

@schell Now, in terms of type upgrades on the Naga side, the default seems to be R+W, with NonReadable and NonWritable removing one of the sides. So it's almost a bit like subtyping? In that the variable's decorations are "narrowing" the type to a stricter one, and this needs to be propagated to all accesses of that variable, but that's about it.

At the very least, images can only exist with at most an array around them, so you don't have the same issue as potentially deeply nested types involving atomics etc.

[teoxoy]

tl;dr - I don't know if AccessQualifier can solve the problem.

@teoxoy I previously thought it might be the case that rust-gpu (which I use to generate my SPIR-V shaders) needed to specify the AccessQualifier so that wgpu knew how the image was going to be used. But after cutting a PR into rust-gpu to be able to set the AccessQualifier I found that using AccessQualifier won't pass spir-val as it requires the Kernel capability:

Ah yes, we can't use the Kernel capability.

@schell Now, in terms of type upgrades on the Naga side, the default seems to be R+W, with NonReadable and NonWritable removing one of the sides. So it's almost a bit like subtyping? In that the variable's decorations are "narrowing" the type to a stricter one, and this needs to be propagated to all accesses of that variable, but that's about it.

It looks like naga already supports this:

wgpu/naga/src/front/spv/mod.rs

Lines 5672 to 5675 in 4f1a9b5

	// Storage image types in IR have to contain the access, but not in the SPIR-V.
	// The same image type in SPIR-V can be used (and has to be used) for multiple images.
	// So we copy the type out and apply the variable access decorations.
	let access = dec.flags.to_storage_access();

All decorations must come before all types, so our implementation looks correct.

[schell]

So in that case, does that mean this fix should indeed be made in rust-gpu, @eddyb ?

[eddyb]

You need both spirv_std::Image support (that gets turned into the NonReadable/NonWritable decorations) and Naga support for those decorations.

The latter existing means you only need the former. The implementation guide on the Rust-GPU side is EmbarkStudios/rust-gpu#1097 (comment) (specificially the Image!(..., non_readable) bullet-point for the approach that would let us gate methods on readability/writability, which seems preferrable)

[teoxoy]

I will close this issue then, please reopen it or open a new one if there is an issue with our implementation.