Repository: KhronosGroup/SPIRV-Tools (language: python)

Issue:
False positive validation for combined image sampler with comparison
I'm trying to rev toolchain dependencies in SPIRV-Cross, and ran into a weird validation error that started appearing since last time I revved the toolchain.

GLSL:
```
#version 450

layout(set = 0, binding = 0) uniform texture2D uTex;
layout(set = 0, binding = 1) uniform sampler uSamp;
layout(location = 0) in vec2 vUV;
layout(location = 0) out float FragColor;

void main()
{
	FragColor = textureLod(sampler2DShadow(uTex, uSamp), vec3(vUV, 0.5), 0.0);
}
```

ASM:
```
; SPIR-V
; Version: 1.0
; Generator: Khronos Glslang Reference Front End; 11
; Bound: 32
; Schema: 0
               OpCapability Shader
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %main "main" %FragColor %vUV
               OpExecutionMode %main OriginUpperLeft
               OpSource GLSL 450
               OpName %main "main"
               OpName %FragColor "FragColor"
               OpName %uTex "uTex"
               OpName %uSamp "uSamp"
               OpName %vUV "vUV"
               OpDecorate %FragColor Location 0
               OpDecorate %uTex DescriptorSet 0
               OpDecorate %uTex Binding 0
               OpDecorate %uSamp DescriptorSet 0
               OpDecorate %uSamp Binding 1
               OpDecorate %vUV Location 0
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
      %float = OpTypeFloat 32
%_ptr_Output_float = OpTypePointer Output %float
  %FragColor = OpVariable %_ptr_Output_float Output
          %9 = OpTypeImage %float 2D 0 0 0 1 Unknown
%_ptr_UniformConstant_9 = OpTypePointer UniformConstant %9
       %uTex = OpVariable %_ptr_UniformConstant_9 UniformConstant
         %13 = OpTypeSampler
%_ptr_UniformConstant_13 = OpTypePointer UniformConstant %13
      %uSamp = OpVariable %_ptr_UniformConstant_13 UniformConstant
         %17 = OpTypeImage %float 2D 1 0 0 1 Unknown
         %18 = OpTypeSampledImage %17
    %v2float = OpTypeVector %float 2
%_ptr_Input_v2float = OpTypePointer Input %v2float
        %vUV = OpVariable %_ptr_Input_v2float Input
  %float_0_5 = OpConstant %float 0.5
    %v3float = OpTypeVector %float 3
    %float_0 = OpConstant %float 0
       %main = OpFunction %void None %3
          %5 = OpLabel
         %12 = OpLoad %9 %uTex
         %16 = OpLoad %13 %uSamp
         %19 = OpSampledImage %18 %12 %16
         %23 = OpLoad %v2float %vUV
         %26 = OpCompositeExtract %float %23 0
         %27 = OpCompositeExtract %float %23 1
         %28 = OpCompositeConstruct %v3float %26 %27 %float_0_5
         %30 = OpCompositeExtract %float %28 2
         %31 = OpImageSampleDrefExplicitLod %float %19 %28 %30 Lod %float_0
               OpStore %FragColor %31
               OpReturn
               OpFunctionEnd
```

spirv-val commit `b21dda0ee7a3ea4e0192a7b2b09db1df1de9d5e7`:
```
error: line 41: Expected Image to have the same type as Result Type Image
  %19 = OpSampledImage %18 %12 %16
```

The problem happens in `sampler2DShadow(tex, samp)` where we now know the combined image sampler is a depth texture. I can't find any wording in the SPIR-V or Vulkan specs which state that the image type for OpTypeCombinedSampler must match the type of the image exactly.

Where in the spec does it say this? If so, that is a problem because otherwise, separate image samplers are not really possible to implement since an image can be used both as comparison sampler and non-comparison sampler in the same shader. Also, array of descriptors has to be considered where index 0 may be a depth texture and index 1 is not.


Discussion / hints:
@alan-baker I think it's this commit: 70ad4dae7dc2c2d035c288b4e9ada0d7bcc26e9b. There is no reference to spec language in this commit.

This is breaking glslang output which has been used since day 1, which is a good indication this commit should be reverted.
The description of OpSampledImage says:

> Result Type must be the [OpTypeSampledImage](https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpTypeSampledImage) type whose Image Type operand is the type of Image.

https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpSampledImage

In this case, the image type changes because they have different depth:

```
          %9 = OpTypeImage %float 2D 0 0 0 1 Unknown
         %17 = OpTypeImage %float 2D 1 0 0 1 Unknown
         %18 = OpTypeSampledImage %17 
        %12 = OpLoad %9 %uTex
         %19 = OpSampledImage %18 %12 %16
```


Filed an internal spec issue to relax this. Trying to validate for different Depth is the problem here, and "no-one" follows that rule, for good reason. Breaking the shader ecosystem this late is not ideal.
SPIR WG discussed this in the 2024-09-04 telecon. Decision was to relax the specific to allow the depth operand to mismatch due to the large existing corpus of shaders in violation of this rule.

Two steps for SPIRV-Tools:
1. revert the original PR
2. add validation that checks all the other operands of the image

Fix patch:
diff --git a/source/val/validate_image.cpp b/source/val/validate_image.cpp
index aadc264e02..faf661afb7 100644
--- a/source/val/validate_image.cpp
+++ b/source/val/validate_image.cpp
@@ -1005,8 +1005,7 @@ bool IsAllowedSampledImageOperand(spv::Op opcode, ValidationState_t& _) {
 
 spv_result_t ValidateSampledImage(ValidationState_t& _,
                                   const Instruction* inst) {
-  auto type_inst = _.FindDef(inst->type_id());
-  if (type_inst->opcode() != spv::Op::OpTypeSampledImage) {
+  if (_.GetIdOpcode(inst->type_id()) != spv::Op::OpTypeSampledImage) {
     return _.diag(SPV_ERROR_INVALID_DATA, inst)
            << "Expected Result Type to be OpTypeSampledImage.";
   }
@@ -1017,11 +1016,6 @@ spv_result_t ValidateSampledImage(ValidationState_t& _,
            << "Expected Image to be of type OpTypeImage.";
   }
 
-  if (type_inst->GetOperandAs<uint32_t>(1) != image_type) {
-    return _.diag(SPV_ERROR_INVALID_DATA, inst)
-           << "Expected Image to have the same type as Result Type Image";
-  }
-
   ImageTypeInfo info;
   if (!GetImageTypeInfo(_, image_type, &info)) {
     return _.diag(SPV_ERROR_INVALID_DATA, inst)


Test patch:
diff --git a/test/val/val_image_test.cpp b/test/val/val_image_test.cpp
index e39ee36f37..77b042f04c 100644
--- a/test/val/val_image_test.cpp
+++ b/test/val/val_image_test.cpp
@@ -4451,7 +4451,7 @@ TEST_F(ValidateImage, QuerySizeNotImage) {
   const std::string body = R"(
 %img = OpLoad %type_image_f32_2d_0011 %uniform_image_f32_2d_0011
 %sampler = OpLoad %type_sampler %uniform_sampler
-%simg = OpSampledImage %type_sampled_image_f32_2d_0011 %img %sampler
+%simg = OpSampledImage %type_sampled_image_f32_2d_0001 %img %sampler
 %res1 = OpImageQuerySize %u32vec2 %sampler
 )";
 
@@ -4465,7 +4465,7 @@ TEST_F(ValidateImage, QuerySizeSampledImageDirectly) {
   const std::string body = R"(
 %img = OpLoad %type_image_f32_2d_0011 %uniform_image_f32_2d_0011
 %sampler = OpLoad %type_sampler %uniform_sampler
-%simg = OpSampledImage %type_sampled_image_f32_2d_0011 %img %sampler
+%simg = OpSampledImage %type_sampled_image_f32_2d_0001 %img %sampler
 %res1 = OpImageQuerySize %u32vec2 %simg
 )";
 
@@ -10647,44 +10647,6 @@ TEST_F(ValidateImage, ImageMSArray_ArrayedTypeDoesNotRequireCapability) {
   EXPECT_THAT(getDiagnosticString(), Eq(""));
 }
 
-TEST_F(ValidateImage, SampledImageTypeMismatch) {
-  const std::string code = R"(
-OpCapability Shader
-OpMemoryModel Logical GLSL450
-OpEntryPoint GLCompute %main "main"
-OpExecutionMode %main LocalSize 1 1 1
-OpDecorate %im_var DescriptorSet 0
-OpDecorate %im_var Binding 0
-OpDecorate %s_var DescriptorSet 1
-OpDecorate %s_var Binding 0
-%void = OpTypeVoid
-%float = OpTypeFloat 32
-%im1_ty = OpTypeImage %float 2D 0 0 0 1 Unknown
-%im2_ty = OpTypeImage %float 2D 1 0 0 1 Unknown
-%s_ty = OpTypeSampler
-%s_im_ty = OpTypeSampledImage %im2_ty
-%ptr_im = OpTypePointer UniformConstant %im1_ty
-%ptr_s = OpTypePointer UniformConstant %s_ty
-%im_var = OpVariable %ptr_im UniformConstant
-%s_var = OpVariable %ptr_s UniformConstant
-%void_fn = OpTypeFunction %void
-%main = OpFunction %void None %void_fn
-%entry = OpLabel
-%im_ld = OpLoad %im1_ty %im_var
-%s_ld = OpLoad %s_ty %s_var
-%sampled_image = OpSampledImage %s_im_ty %im_ld %s_ld
-OpReturn
-OpFunctionEnd
-)";
-
-  const spv_target_env env = SPV_ENV_VULKAN_1_0;
-  CompileSuccessfully(code, env);
-  ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions(env));
-  EXPECT_THAT(
-      getDiagnosticString(),
-      HasSubstr("Expected Image to have the same type as Result Type Image"));
-}
-
 }  // namespace
 }  // namespace val
 }  // namespace spvtools