EmbarkStudios / rust-gpu

🐉 Making Rust a first-class language and ecosystem for GPU shaders 🚧
https://shader.rs
Apache License 2.0
7.28k stars 246 forks source link

Image macro doesn't fully enforce valid use of non-sampled storage images #812

Open fu5ha opened 2 years ago

fu5ha commented 2 years ago

Currently, if we create an image type with the image macro as such:

Image!(2D, type=f32, sampled=false),

the image will still be able to be .read/.fetched, and the spirv Image object won't be decorated as NonReadable. However, according to vulkan spec, "If shaderStorageImageReadWithoutFormat is not enabled, any variable created with a "Type" of OpTypeImage that has a "Sampled" operand of 2 and an "Image Format" operand of Unknown must be decorated with NonReadable."

This can be solved for now by just using an explicit format=blah in the Image macro instead of type, but I think we could also handle this/validate it in the macro automatically. I think the best way forward would be:

khyperia commented 2 years ago

Just for some context here, I've looked into adding NonReadable before and there's some issues with the spec that I would like clarified before implementing anything - I don't want to make guesses on what the "intended" behavior is.

Feel free to ping me if anyone would like to look into those issues and drive that discussion with khronos - for example, one issue is that it's unspecified behavior when an object decorated with NonReadable is passed to a function whose argument is not decorated with NonReadable (as the decoration is on values, not types)

khyperia commented 2 years ago

Just did some investigation (made annoying by the fact that my little wgpu test loader doesn't seem to trigger the validation error), and it seems like while spirv-val is silent about everything related to this issue, vulkan validation layers behaves like so. I have a helper function that takes a storage image pointer and OpImageWrites some dummy value inside the function.

So, it looks like validation layers only cares about the global OpVariable being decorated with NonReadable, and is totes fine with garbage annotations on functions where that global OpVariable is passed into. I am guessing this is a quirk of validation layers, and not intended behavior (especially considering this error was implemented in validation layers extremely recently).

As an aside, using glslang, with variants on this shader, this is the result:

#version 460
layout (binding = 0) uniform writeonly image2D tex;
void f(writeonly image2D t) { imageStore(t, ivec2(0, 0), vec4(0)); }
void main() { f(tex); }

Meanwhile, this program compiles

#version 460
layout (binding = 0, rgba32f) uniform image2D tex;
void f(writeonly image2D t) { imageStore(t, ivec2(0, 0), vec4(0)); }
void main() { f(tex); }

while this one does not ("image format arguments must match" - which uh, what? how does the above compile but this doesn't? some context, you can't specify image formats on function parameters)

#version 460
layout (binding = 0, rgba32f) uniform image2D tex;
void f(image2D t) { imageStore(t, ivec2(0, 0), vec4(0)); }
void main() { f(tex); }

So, assuming the (not without precedent) that SPIR-V intended to copy GLSL here, that means:

I assume a similar investigation should happen for NonWritable.