KhronosGroup / SPIRV-Tools

Apache License 2.0
1.09k stars 556 forks source link

spirv-val: Incorrect validation of NonWritable decoration #3322

Open StuartDBrady opened 4 years ago

StuartDBrady commented 4 years ago

The OpenCL CTS includes a set of pre-assembled SPIR-V modules, as part of its "spirv_new" test component. Two of the binaries fail to validate, but it appears that this may be due to a bug in the validator. The failures are:

$ spirv-val test_conformance/spirv_new/spirv_bin/decorate_nonwritable.spv32
error: line 57: Target of NonWritable decoration is invalid: must point to a storage image, uniform block, or storage buffer
  %src = OpFunctionParameter %28
$ spirv-val test_conformance/spirv_new/spirv_bin/decorate_nonwritable.spv64
error: line 65: Target of NonWritable decoration is invalid: must point to a storage image, uniform block, or storage buffer
  %src = OpFunctionParameter %35

In each case, the decoration is applied to an OpFunctionParameter of type OpTypeImage %void 2D 0 0 0 0 Unknown ReadOnly (i.e. %28 and %35). Here, the sampler type of 0 indicates that "whether or not this image will be accessed in combination with a simpler" "is only known at run time, not at compile time". Changing this to 2 silences this validation failure, but replaces it with another one.

If the sampler type is changed to 2, then the subsequent use of OpSampledImage fails to validate. The OpSampledImage instruction in question takes a result type which is the result of OpTypeSampledImage, applied to the OpTypeImage type described above. The image operand in the previously described OpFunctionParameter result. The validation failures are then as follows:

$ spirv-val test_conformance/spirv_new/spirv_bin/decorate_nonwritable.spv32
error: line 70: Expected Image 'Sampled' parameter to be 0 or 1
  %TempSampledImage = OpSampledImage %32 %src %34
$ spirv-val test_conformance/spirv_new/spirv_bin/decorate_nonwritable.spv64
error: line 81: Expected Image 'Sampled' parameter to be 0 or 1
  %TempSampledImage = OpSampledImage %39 %src %41

The SPIR-V specification states that for OpSampledImage:

Image is an object whose type is an OpTypeImage, whose Sampled operand is 0 or 1, and whose Dim operand is not SubpassData.

Presumably, a Sampler type in OpTypeImage of 2 indicates that the image will never be used with a sampler, not even with OpSampledImage.

Is the validator therefore being too strict in disallowing use of NonWritable against images with runtime determination of sampling? It seems to me that the intent of the specification was to forbid use of NonWritable with sampled images, as these are inherently non-writable.

If so, the validator should accept use of the NonWritable decoration on images for which the image type has a Sampling operand of 0.

StuartDBrady commented 4 years ago

Note also, the decorate_volatile.spv{32,64} and decorate_coherent.spv{32,64} tests pass despite being almost identical to the decorate_nonwritable tests, and despite the Volatile and Coherent decorations having very similar requirements with regards to storage images. It seems plausible that there is a gap in validation coverage for the Volatile and Coherent decorations.

alan-baker commented 4 years ago

The spec for NonWritable says:

Must be applied only to memory object declarations or members of a structure type. Any such memory object declaration, or any memory object declaration that contains such a structure type, must be one of:

  • A storage image (see OpTypeImage).
  • ...

And a storage image is defined in OpTypeImage as:

Sampled indicates whether or not this image will be accessed in combination with a sampler, and must be one of the following values: 0 indicates this is only known at run time, not at compile time 1 indicates will be used with sampler 2 indicates will be used without a sampler (a storage image)

So the spec seems to indicate the decoration should not be used with OpenCL images. I would suggest opening on internal SPIR-V issue about this.

StuartDBrady commented 4 years ago

Why does it follow that the decoration should not be used with OpenCL images? OpenCL images support samplers. What have I missed?

alan-baker commented 4 years ago

The NonWritable decoration can only be applied to storage images. That is images with a Sampled value of 2. OpenCL uses images with a Sampled value of 0. Those are not storage images according to the spec.