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

Issue:
spirv-val: Small Cleanup of validate image
Have some other Vulkan changes related to images, but pulled this out as a separate PR

- `ValidateImageCommon` seemed to be a random function used only sometimes that I just broke into 2 dedicated functions
- grouped the checks in `ValidateTypeImage` to be Universal, CL, then Vulkan checks
- grouped CL and Vulkan checks together where it makes sense inside given functions


Discussion / hints:

Fix patch:
diff --git a/source/val/validate_image.cpp b/source/val/validate_image.cpp
index 7c8dfee765..39eeb4bd7e 100644
--- a/source/val/validate_image.cpp
+++ b/source/val/validate_image.cpp
@@ -693,16 +693,11 @@ spv_result_t ValidateImageReadWrite(ValidationState_t& _,
              << "storage image";
     }
 
-    if (info.multisampled == 1 &&
+    if (info.multisampled == 1 && info.arrayed == 1 && info.sampled == 2 &&
         !_.HasCapability(spv::Capability::ImageMSArray)) {
-#if 0
-      // TODO(atgoo@github.com) The description of this rule in the spec
-      // is unclear and Glslang doesn't declare ImageMSArray. Need to clarify
-      // and reenable.
-      return _.diag(SPV_ERROR_INVALID_DATA, inst)
-          << "Capability ImageMSArray is required to access storage "
-          << "image";
-#endif
+      return _.diag(SPV_ERROR_INVALID_DATA, inst)
+             << "Capability ImageMSArray is required to access storage "
+             << "image";
     }
   } else if (info.sampled != 0) {
     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 cf317ec989..9a704098de 100644
--- a/test/val/val_image_test.cpp
+++ b/test/val/val_image_test.cpp
@@ -7936,6 +7936,116 @@ TEST_F(ValidateImage, QCOMImageProcessingSampleWeightedInvalidUseB) {
       HasSubstr("Illegal use of QCOM image processing decorated texture"));
 }
 
+TEST_F(ValidateImage, ImageMSArray_ArrayedSampledTypeRequiresCapability) {
+  const std::string code = R"(
+               OpCapability Shader
+               OpCapability StorageImageMultisample
+               OpCapability StorageImageReadWithoutFormat
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main"
+               OpExecutionMode %main OriginUpperLeft
+               OpDecorate %var_image DescriptorSet 0
+               OpDecorate %var_image Binding 1
+       %void = OpTypeVoid
+       %func = OpTypeFunction %void
+        %f32 = OpTypeFloat 32
+        %u32 = OpTypeInt 32 0
+     %uint_2 = OpConstant %u32 2
+     %uint_1 = OpConstant %u32 1
+     %v2uint = OpTypeVector %u32 2
+    %v4float = OpTypeVector %f32 4
+    %image = OpTypeImage %f32 2D 2 1 1 2 Unknown
+%ptr_image = OpTypePointer UniformConstant %image
+       %10 = OpConstantComposite %v2uint %uint_1 %uint_2
+%var_image = OpVariable %ptr_image UniformConstant
+     %main = OpFunction %void None %func
+ %main_lab = OpLabel
+       %18 = OpLoad %image %var_image
+       %19 = OpImageRead %v4float %18 %10 Sample %uint_2
+             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("Capability ImageMSArray is required to access storage image"));
+}
+
+TEST_F(ValidateImage, ImageMSArray_SampledTypeDoesNotRequireCapability) {
+  const std::string code = R"(
+               OpCapability Shader
+               OpCapability StorageImageMultisample
+               OpCapability StorageImageReadWithoutFormat
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main"
+               OpExecutionMode %main OriginUpperLeft
+               OpDecorate %var_image DescriptorSet 0
+               OpDecorate %var_image Binding 1
+       %void = OpTypeVoid
+       %func = OpTypeFunction %void
+        %f32 = OpTypeFloat 32
+        %u32 = OpTypeInt 32 0
+     %uint_2 = OpConstant %u32 2
+     %uint_1 = OpConstant %u32 1
+     %v2uint = OpTypeVector %u32 2
+    %v4float = OpTypeVector %f32 4
+    %image = OpTypeImage %f32 2D 2 0 1 2 Unknown
+%ptr_image = OpTypePointer UniformConstant %image
+       %10 = OpConstantComposite %v2uint %uint_1 %uint_2
+%var_image = OpVariable %ptr_image UniformConstant
+     %main = OpFunction %void None %func
+ %main_lab = OpLabel
+       %18 = OpLoad %image %var_image
+       %19 = OpImageRead %v4float %18 %10 Sample %uint_2
+             OpReturn
+             OpFunctionEnd
+)";
+
+  const spv_target_env env = SPV_ENV_VULKAN_1_0;
+  CompileSuccessfully(code, env);
+  ASSERT_EQ(SPV_SUCCESS, ValidateInstructions(env));
+  EXPECT_THAT(getDiagnosticString(), Eq(""));
+}
+
+TEST_F(ValidateImage, ImageMSArray_ArrayedTypeDoesNotRequireCapability) {
+  const std::string code = R"(
+               OpCapability Shader
+               OpCapability StorageImageReadWithoutFormat
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main"
+               OpExecutionMode %main OriginUpperLeft
+               OpDecorate %var_image DescriptorSet 0
+               OpDecorate %var_image Binding 1
+       %void = OpTypeVoid
+       %func = OpTypeFunction %void
+        %f32 = OpTypeFloat 32
+        %u32 = OpTypeInt 32 0
+     %uint_3 = OpConstant %u32 3
+     %uint_2 = OpConstant %u32 2
+     %uint_1 = OpConstant %u32 1
+     %v3uint = OpTypeVector %u32 3
+    %v4float = OpTypeVector %f32 4
+    %image = OpTypeImage %f32 2D 2 1 0 2 Unknown
+%ptr_image = OpTypePointer UniformConstant %image
+       %10 = OpConstantComposite %v3uint %uint_1 %uint_2 %uint_3
+%var_image = OpVariable %ptr_image UniformConstant
+     %main = OpFunction %void None %func
+ %main_lab = OpLabel
+       %18 = OpLoad %image %var_image
+       %19 = OpImageRead %v4float %18 %10
+             OpReturn
+             OpFunctionEnd
+)";
+
+  const spv_target_env env = SPV_ENV_VULKAN_1_0;
+  CompileSuccessfully(code, env);
+  ASSERT_EQ(SPV_SUCCESS, ValidateInstructions(env));
+  EXPECT_THAT(getDiagnosticString(), Eq(""));
+}
+
 }  // namespace
 }  // namespace val
 }  // namespace spvtools