Invalid SPIR-V Generated For samplerBuffer in SPIR-V 1.6

[qingyuanzNV]

In SPIR-V 1.6 spec, it is no longer valid to create a sampled image with dimension of buffer. Specifically, quote the following spec text:

OpTypeSampledImage
Declare a sampled image type, the Result Type of OpSampledImage, or an externally combined sampler
and image. This type is opaque: values of this type have no defined physical size or bit pattern.
Image Type must be an OpTypeImage. It is the type of the image in the combined sampler and image
type. It must not have a Dim of SubpassData. Additionally, starting with version 1.6, it must not have a
Dim of Buffer.

However, in glslang, we'll still emit OpTypeSampledImage and OpSampledImage for samplerBuffer. Translation of samplerBuffer into SPIR-V needs to be clarified and fixed.

One possible fix could be: b3e8969

[greg-lunarg]

I get what you are doing with b3e8969. Essentially, if someone tries to combine a sampler to a buffer, ignore (throw away) the sampler and keep going. The sampler is never going to be needed anyway.

It doesn't solve the problem if the user declares a samplerBuffer targeting spirv1.6, but maybe that can be a different issue.

[qingyuanzNV]

If user declares a samplerBuffer, the fix is at line 4161 so we don't make it a sampledImage.
However, this implementation feels a little hacky to me. I suppose the spec should eventually remove samplerBuffer.

[greg-lunarg]

However, this implementation feels a little hacky to me.

Yes. My concern is to silently accept the samplerBuffer but just create a buffer in the spirv, which would not match what the user is expecting.

Then, if we are not supporting samplerBuffer from first principles, I do not feel right about allowing it to be created with a constructor.

It feels to me the solution is to give an error that samplerBuffer is not supported for spirv1.6.

@gnl21 What do you think of giving such an error?

[qingyuanzNV]

However, this implementation feels a little hacky to me.

Yes. My concern is to silently accept the samplerBuffer but just create a buffer in the spirv, which would not match what the user is expecting.

Then, if we are not supporting samplerBuffer from first principles, I do not feel right about allowing it to be created with a constructor.

It feels to me the solution is to give an error that samplerBuffer is not supported for spirv1.6.

@gnl21 What do you think of giving such an error?

As far as I know, both samplerBuffer and textureBuffer are referred as "Uniform Texel Buffers". It looks like samplerBuffer was a workaround for buffering in the OpenGL era, which couldn't be sampled anyway. In vulkan context, they almost can be used interchangeably. I could be wrong though.

[gnl21]

The change in SPIR-V was motivated by the fact that it was never possible to use a buffer with a sampler, it was just a quirk of the fact that OpenGL had combined image/sampler types, so it was natural to declare it that way. With that in mind, I would say that samplerBuffer and textureBuffer are functionally equivalent and have always been. Thus translating a samplerBuffer to just an image with dim=Buffer seems fine to me -- it doesn't differ from what the user was expecting because it is equivalent.

I don't think it makes sense to define user expectations in terms of the exact SPIR-V produced, since optimisations, etc. can change it. The compiler is producing a valid and sensible translation of the GLSL in this case when it drops the sampler. In this case, since any use of the OpSampledImage would have to have the image extracted using OpImage first, we're just baking in an obvious optimisation.

I think a warning would be appropriate if the input specifically has a constructor creating a samplerBuffer from a textureBuffer, but I'm concerned that making it an error needlessly restricts backwards compatibility. We already accept code like this, so we can't really ban it from all versions of GLSL, and I don't think that having the language change depending on the version of SPIR-V that you're compiling for is a particularly good idea.

[greg-lunarg]

@gnl21 If indeed you think it is acceptable to generate a textureBuffer in SPIR-V for a GLSL samplerBuffer going forward, that works for me. Thanks for your perspective!

Given this, I will move ahead with the review.