spirv-reader: Restrict use of ConstOffset

Only permitted for image sampling (and later gather).

Only permitted for 2D, 2D Array, and 3D textures

Fixed: tint:408
Change-Id: Ib97bd17e45046ec8a2147b46899bf52ad9a8f883
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/35980
Auto-Submit: David Neto <dneto@google.com>
Commit-Queue: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: Ryan Harrison <rharrison@chromium.org>

src/reader/spirv/function.cc

@@ -500,6 +500,22 @@ bool IsSampledImageAccess(SpvOp opcode) {
   return false;
 }
 
+// @param opcode a SPIR-V opcode
+// @returns true if the given instruction is an image sampling operation.
+bool IsImageSampling(SpvOp opcode) {
+  switch (opcode) {
+    case SpvOpImageSampleImplicitLod:
+    case SpvOpImageSampleExplicitLod:
+    case SpvOpImageSampleDrefImplicitLod:
+    case SpvOpImageSampleDrefExplicitLod:
+      return true;
+    default:
+      // WGSL doesn't have *Proj* texturing.
+      break;
+  }
+  return false;
+}
+
 // @param opcode a SPIR-V opcode
 // @returns true if the given instruction is an image access instruction
 // whose first input operand is an OpImage value.
@@ -3989,7 +4005,8 @@ bool FunctionEmitter::EmitImageAccess(const spvtools::opt::Instruction& inst) {
   params.push_back(create<ast::IdentifierExpression>(
       Source{}, ast_module_.RegisterSymbol(name), name));
 
-  if (IsSampledImageAccess(inst.opcode())) {
+  const auto opcode = inst.opcode();
+  if (IsSampledImageAccess(opcode)) {
     // Form the sampler operand.
     const auto* sampler = parser_impl_.GetMemoryObjectDeclarationForHandle(
         image_or_sampled_image_operand_id, false);
@@ -4029,7 +4046,7 @@ bool FunctionEmitter::EmitImageAccess(const spvtools::opt::Instruction& inst) {
 
   std::string builtin_name;
   bool use_load_suffix = true;
-  switch (inst.opcode()) {
+  switch (opcode) {
     case SpvOpImageSampleImplicitLod:
     case SpvOpImageSampleExplicitLod:
       builtin_name = "textureSample";
@@ -4121,12 +4138,28 @@ bool FunctionEmitter::EmitImageAccess(const spvtools::opt::Instruction& inst) {
   }
   if (arg_index < num_args &&
       (image_operands_mask & SpvImageOperandsConstOffsetMask)) {
+    if (!IsImageSampling(opcode)) {
+      return Fail() << "ConstOffset is only permitted for sampling operations: "
+                    << inst.PrettyPrint();
+    }
+    switch (texture_type->dim()) {
+      case ast::type::TextureDimension::k2d:
+      case ast::type::TextureDimension::k2dArray:
+      case ast::type::TextureDimension::k3d:
+        break;
+      default:
+        return Fail() << "ConstOffset is only permitted for 2D, 2D Arrayed, "
+                         "and 3D textures: "
+                      << inst.PrettyPrint();
+    }
+
     params.push_back(ToSignedIfUnsigned(MakeOperand(inst, arg_index)).expr);
     image_operands_mask ^= SpvImageOperandsConstOffsetMask;
     arg_index++;
   }
   if (arg_index < num_args &&
       (image_operands_mask & SpvImageOperandsSampleMask)) {
+    // TODO(dneto): only permitted with ImageFetch
     params.push_back(ToI32(MakeOperand(inst, arg_index)).expr);
     image_operands_mask ^= SpvImageOperandsSampleMask;
     arg_index++;
@@ -4184,7 +4217,7 @@ bool FunctionEmitter::EmitImageAccess(const spvtools::opt::Instruction& inst) {
       value = create<ast::BitcastExpression>(Source{}, result_type, call_expr);
     }
     if (!expected_component_type->Is<ast::type::F32>() &&
-        IsSampledImageAccess(inst.opcode())) {
+        IsSampledImageAccess(opcode)) {
       // WGSL permits sampled image access only on float textures.
       // Reject this case in the SPIR-V reader, at least until SPIR-V validation
       // catches up with this rule and can reject it earlier in the workflow.

src/reader/spirv/parser_impl_handle_test.cc

@@ -30,6 +30,7 @@ namespace {
 using ::testing::Eq;
 using ::testing::HasSubstr;
 using ::testing::Not;
+using ::testing::StartsWith;
 
 std::string Preamble() {
   return R"(
@@ -2617,28 +2618,6 @@ INSTANTIATE_TEST_SUITE_P(
         Identifier[not set]{vi12}
         Identifier[not set]{vf1234}
       )
-    })"},
-        // OpImageWrite with ConstOffset
-        {"%float 2D 0 0 0 2 Rgba32f",
-         "OpImageWrite %im %vi12 %vf1234 ConstOffset %offsets2d",
-         R"(
-  Variable{
-    Decorations{
-      SetDecoration{2}
-      BindingDecoration{1}
-    }
-    x_20
-    uniform_constant
-    __storage_texture_write_only_2d_rgba32float
-  })",
-         R"(Call[not set]{
-      Identifier[not set]{textureStore}
-      (
-        Identifier[not set]{x_20}
-        Identifier[not set]{vi12}
-        Identifier[not set]{vf1234}
-        Identifier[not set]{offsets2d}
-      )
     })"}}));
 
 INSTANTIATE_TEST_SUITE_P(
@@ -3085,35 +3064,6 @@ INSTANTIATE_TEST_SUITE_P(
           }
         }
       }
-    })"},
-        // OpImageRead with ConstOffset
-        {"%float 2D 0 0 0 2 Rgba32f",
-         "%99 = OpImageRead %v4float %im %vi12 ConstOffset %offsets2d",
-         R"(Variable{
-    Decorations{
-      SetDecoration{2}
-      BindingDecoration{1}
-    }
-    x_20
-    uniform_constant
-    __storage_texture_read_only_2d_rgba32float
-  })",
-         R"(VariableDeclStatement{
-      VariableConst{
-        x_99
-        none
-        __vec_4__f32
-        {
-          Call[not set]{
-            Identifier[not set]{textureLoad}
-            (
-              Identifier[not set]{x_20}
-              Identifier[not set]{vi12}
-              Identifier[not set]{offsets2d}
-            )
-          }
-        }
-      }
     })"}}));
 
 INSTANTIATE_TEST_SUITE_P(
@@ -3146,36 +3096,6 @@ INSTANTIATE_TEST_SUITE_P(
           }
         }
       }
-    })"},
-        // OpImageFetch with ConstOffset
-        // TODO(dneto): Seems this is not valid in WGSL.
-        {"%float 2D 0 0 0 1 Unknown",
-         "%99 = OpImageFetch %v4float %im %vi12 ConstOffset %offsets2d",
-         R"(Variable{
-    Decorations{
-      SetDecoration{2}
-      BindingDecoration{1}
-    }
-    x_20
-    uniform_constant
-    __sampled_texture_2d__f32
-  })",
-         R"(VariableDeclStatement{
-      VariableConst{
-        x_99
-        none
-        __vec_4__f32
-        {
-          Call[not set]{
-            Identifier[not set]{textureLoad}
-            (
-              Identifier[not set]{x_20}
-              Identifier[not set]{vi12}
-              Identifier[not set]{offsets2d}
-            )
-          }
-        }
-      }
     })"},
         // OpImageFetch with explicit level
         {"%float 2D 0 0 0 1 Unknown",
@@ -3760,7 +3680,7 @@ TEST_P(SpvParserTest_ImageCoordsTest, MakeCoordinateOperandsForImageAccess) {
   )";
   auto p = parser(test::Assemble(assembly));
   if (!p->BuildAndParseInternalModule()) {
-    EXPECT_THAT(p->error(), Eq(GetParam().expected_error)) << assembly;
+    EXPECT_THAT(p->error(), StartsWith(GetParam().expected_error)) << assembly;
   } else {
     EXPECT_TRUE(p->error().empty()) << p->error();
     FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
@@ -4456,6 +4376,59 @@ INSTANTIATE_TEST_SUITE_P(
          "sampled image must have float component type",
          {}}}));
 
+INSTANTIATE_TEST_SUITE_P(
+    ConstOffset_BadInstruction_Errors,
+    SpvParserTest_ImageCoordsTest,
+    ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+        // ImageFetch
+        {"%uint 2D 0 0 0 1 Unknown",
+         "%result = OpImageFetch %v4uint %sampled_image %vf12 ConstOffset "
+         "%the_vu12",
+         "ConstOffset is only permitted for sampling operations: ",
+         {}},
+        // ImageRead
+        {"%uint 2D 0 0 0 2 Rgba32ui",
+         "%result = OpImageRead %v4uint %im %vu12 ConstOffset %the_vu12",
+         "ConstOffset is only permitted for sampling operations: ",
+         {}},
+        // ImageWrite
+        {"%uint 2D 0 0 0 2 Rgba32ui",
+         "OpImageWrite %im %vu12 %vu1234 ConstOffset %the_vu12",
+         "ConstOffset is only permitted for sampling operations: ",
+         {}}
+        // TODO(dneto): Gather
+        // TODO(dneto): DrefGather
+    }));
+
+INSTANTIATE_TEST_SUITE_P(
+    ConstOffset_BadDim_Errors,
+    SpvParserTest_ImageCoordsTest,
+    ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+        // 1D
+        {"%uint 1D 0 0 0 1 Unknown",
+         "%result = OpImageSampleImplicitLod %v4float %sampled_image %vf1234 "
+         "ConstOffset %the_vu12",
+         "ConstOffset is only permitted for 2D, 2D Arrayed, and 3D textures: ",
+         {}},
+        // 1D Array
+        {"%uint 1D 0 1 0 1 Unknown",
+         "%result = OpImageSampleImplicitLod %v4float %sampled_image %vf1234 "
+         "ConstOffset %the_vu12",
+         "ConstOffset is only permitted for 2D, 2D Arrayed, and 3D textures: ",
+         {}},
+        // Cube
+        {"%uint Cube 0 0 0 1 Unknown",
+         "%result = OpImageSampleImplicitLod %v4float %sampled_image %vf1234 "
+         "ConstOffset %the_vu12",
+         "ConstOffset is only permitted for 2D, 2D Arrayed, and 3D textures: ",
+         {}},
+        // Cube Array
+        {"%uint Cube 0 1 0 1 Unknown",
+         "%result = OpImageSampleImplicitLod %v4float %sampled_image %vf1234 "
+         "ConstOffset %the_vu12",
+         "ConstOffset is only permitted for 2D, 2D Arrayed, and 3D textures: ",
+         {}}}));
+
 }  // namespace
 }  // namespace spirv
 }  // namespace reader