rust-gpu icon indicating copy to clipboard operation
rust-gpu copied to clipboard

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

Open fu5ha opened this issue 4 years ago • 2 comments

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:

  • Make a readable flag in the macro which gets auto-disabled under the above conditions, and when disabled, decorates the OpTypeImage with NonReadable and doesn't allow to do .read/.fetch on the Image.
  • However, if it is manually enabled, then we do the current behavior, allowing read etc. (assuming shaderStorageImageReadWithoutFormat is enabled).

fu5ha avatar Nov 29 '21 20:11 fu5ha

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 avatar Nov 29 '21 22:11 khyperia

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.

  • Image OpVariable not marked with NonReadable: error (as this issue reports)
  • Image OpVariable marked with NonReadable: no error, as expected
  • Image OpVariable not marked with NonReadable, calling function whose parameter is not marked with NonReadable: error (as expected)
  • Image OpVariable marked with NonReadable, calling function whose parameter is marked with NonReadable: totally fine, as expected
  • Image OpVariable marked with NonReadable, calling function whose parameter is not marked with NonReadable: totally fine, according to validation layers!
  • Image OpVariable not marked with NonReadable, calling function whose parameter is marked with NonReadable: error around "thing needs NonReadable pls", but totally fine with the presumably-sketchy call.

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); }
  • both writeonly: fine
  • neither writeonly: error, "must be declared writeonly, or have a format"
  • global writeonly, function without: error, "argument cannot drop memory qualifier when passed to formal parameter"
  • global without, function writeonly: error, "must be declared writeonly, or have a format"

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:

  • There should be a SPIR-V rule saying you cannot call a function with a variable marked as NonReadable to a function whose parameter is not marked as NonReadable
  • There should be a SPIR-V rule saying that you can call a function with a variable with no decoration to a function whose parameter is marked as NonReadable
  • There should be a SPIR-V rule saying you can call a function whose parameter has image format=unknown with an image whose format is known (haven't dug through the spec too much to check if this is already present). Maybe the other way around, too, but AFAIK that's not a constructable situation in GLSL.

I assume a similar investigation should happen for NonWritable.

khyperia avatar Nov 30 '21 14:11 khyperia