From 37c99ab7e5a71bdae88a1e7b4db74c61ea13313f Mon Sep 17 00:00:00 2001
From: Jaebaek Seo <jaebaek@google.com>
Date: Tue, 18 Sep 2018 11:41:29 -0400
Subject: [PATCH] Validator: OpImageQuerySize validation (#1538)

Validation of OpImageQuerySize is missing that is a TODO. This
commit implements its validation based on the spec.
---
 source/val/validate_image.cpp | 30 +++++++-----------------
 test/val/val_image_test.cpp   | 44 +++++++++++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 25 deletions(-)

diff --git a/source/val/validate_image.cpp b/source/val/validate_image.cpp
index 83482d785..7d35d5633 100644
--- a/source/val/validate_image.cpp
+++ b/source/val/validate_image.cpp
@@ -1474,9 +1474,6 @@ spv_result_t ValidateImageQuerySize(ValidationState_t& _,
            << "Expected Image to be of type OpTypeImage";
   }
 
-#if 0
-  // TODO(atgoo@github.com) The spec doesn't whitelist all Dims supported by
-  // GLSL. Need to verify if there is an error and reenable.
   ImageTypeInfo info;
   if (!GetImageTypeInfo(_, image_type, &info)) {
     return _.diag(SPV_ERROR_INVALID_DATA, inst)
@@ -1485,37 +1482,29 @@ spv_result_t ValidateImageQuerySize(ValidationState_t& _,
 
   uint32_t expected_num_components = info.arrayed;
   switch (info.dim) {
+    case SpvDim1D:
     case SpvDimBuffer:
       expected_num_components += 1;
       break;
     case SpvDim2D:
-      if (info.multisampled != 1 && info.sampled != 0 &&
-          info.sampled != 2) {
-        return _.diag(SPV_ERROR_INVALID_DATA, inst)
-            << "Expected either 'MS'=1 or 'Sampled'=0 or 'Sampled'=2 "
-            << "for 2D dim";
-      }
+    case SpvDimCube:
     case SpvDimRect:
       expected_num_components += 2;
       break;
     case SpvDim3D:
       expected_num_components += 3;
-      if (info.sampled != 0 &&
-          info.sampled != 2) {
-        return _.diag(SPV_ERROR_INVALID_DATA, inst)
-            << "Expected either 'Sampled'=0 or 'Sampled'=2 "
-            << "for 3D dim";
-      }
       break;
     default:
       return _.diag(SPV_ERROR_INVALID_DATA, inst)
-          << "Image 'Dim' must be Buffer, 2D, 3D or Rect";
+             << "Image 'Dim' must be 1D, Buffer, 2D, Cube, 3D or Rect";
   }
 
-
-  if (info.multisampled != 0) {
-    return _.diag(SPV_ERROR_INVALID_DATA, inst)
-        << "Image 'MS' must be 0";
+  if (info.dim == SpvDim1D || info.dim == SpvDim2D || info.dim == SpvDim3D ||
+      info.dim == SpvDimCube) {
+    if (info.multisampled != 1 && info.sampled != 0 && info.sampled != 2) {
+      return _.diag(SPV_ERROR_INVALID_DATA, inst)
+             << "Image must have either 'MS'=1 or 'Sampled'=0 or 'Sampled'=2";
+    }
   }
 
   uint32_t result_num_components = _.GetDimension(result_type);
@@ -1524,7 +1513,6 @@ spv_result_t ValidateImageQuerySize(ValidationState_t& _,
         << "Result Type has " << result_num_components << " components, "
         << "but " << expected_num_components << " expected";
   }
-#endif
 
   return SPV_SUCCESS;
 }
diff --git a/test/val/val_image_test.cpp b/test/val/val_image_test.cpp
index f7e2086a2..5f4783dac 100644
--- a/test/val/val_image_test.cpp
+++ b/test/val/val_image_test.cpp
@@ -3213,7 +3213,7 @@ TEST_F(ValidateImage, QuerySizeLodWrongLodType) {
 
 TEST_F(ValidateImage, QuerySizeSuccess) {
   const std::string body = R"(
-%img = OpLoad %type_image_f32_2d_0001 %uniform_image_f32_2d_0001
+%img = OpLoad %type_image_f32_2d_0010 %uniform_image_f32_2d_0010
 %res1 = OpImageQuerySize %u32vec2 %img
 )";
 
@@ -3223,7 +3223,7 @@ TEST_F(ValidateImage, QuerySizeSuccess) {
 
 TEST_F(ValidateImage, QuerySizeWrongResultType) {
   const std::string body = R"(
-%img = OpLoad %type_image_f32_2d_0001 %uniform_image_f32_2d_0001
+%img = OpLoad %type_image_f32_2d_0010 %uniform_image_f32_2d_0010
 %res1 = OpImageQuerySize %f32vec2 %img
 )";
 
@@ -3236,7 +3236,7 @@ TEST_F(ValidateImage, QuerySizeWrongResultType) {
 
 TEST_F(ValidateImage, QuerySizeNotImage) {
   const std::string body = R"(
-%img = OpLoad %type_image_f32_2d_0001 %uniform_image_f32_2d_0001
+%img = OpLoad %type_image_f32_2d_0010 %uniform_image_f32_2d_0010
 %sampler = OpLoad %type_sampler %uniform_sampler
 %simg = OpSampledImage %type_sampled_image_f32_2d_0001 %img %sampler
 %res1 = OpImageQuerySize %u32vec2 %simg
@@ -3248,7 +3248,43 @@ TEST_F(ValidateImage, QuerySizeNotImage) {
               HasSubstr("Expected Image to be of type OpTypeImage"));
 }
 
-// TODO(atgoo@github.com) Add more tests for OpQuerySize.
+TEST_F(ValidateImage, QuerySizeDimSubpassDataBad) {
+  const std::string body = R"(
+%img = OpLoad %type_image_f32_spd_0002 %uniform_image_f32_spd_0002
+%res1 = OpImageQuerySize %u32vec2 %img
+)";
+
+  CompileSuccessfully(GenerateShaderCode(body).c_str());
+  ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("Image 'Dim' must be 1D, Buffer, 2D, Cube, 3D or Rect"));
+}
+
+TEST_F(ValidateImage, QuerySizeWrongSampling) {
+  const std::string body = R"(
+%img = OpLoad %type_image_f32_2d_0001 %uniform_image_f32_2d_0001
+%res1 = OpImageQuerySize %u32vec2 %img
+)";
+
+  CompileSuccessfully(GenerateKernelCode(body).c_str());
+  ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(
+      getDiagnosticString(),
+      HasSubstr("Image must have either 'MS'=1 or 'Sampled'=0 or 'Sampled'=2"));
+}
+
+TEST_F(ValidateImage, QuerySizeWrongNumberOfComponents) {
+  const std::string body = R"(
+%img = OpLoad %type_image_f32_3d_0111 %uniform_image_f32_3d_0111
+%res1 = OpImageQuerySize %u32vec2 %img
+)";
+
+  CompileSuccessfully(GenerateShaderCode(body).c_str());
+  ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+  EXPECT_THAT(getDiagnosticString(),
+              HasSubstr("Result Type has 2 components, but 4 expected"));
+}
 
 TEST_F(ValidateImage, QueryLodSuccessKernel) {
   const std::string body = R"(