spirv-reader: split off last coordinate for arrayed texture access

Also ensure that the number of texture coordinates is exactly
the right number required for the given texture dimension.
I think SPIR-V is looser in this respect.

Assumes coordinates are floating point.

Bug: tint:349
Change-Id: I4512c333fada3647c66f13ef31897b2d73b46cf0
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/33982
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: David Neto <dneto@google.com>
Auto-Submit: David Neto <dneto@google.com>

src/reader/spirv/function.cc

@@ -16,6 +16,7 @@
 
 #include <algorithm>
 #include <array>
+#include <cassert>
 #include <sstream>
 #include <unordered_map>
 #include <unordered_set>
@@ -53,6 +54,7 @@
 #include "src/ast/switch_statement.h"
 #include "src/ast/type/bool_type.h"
 #include "src/ast/type/pointer_type.h"
+#include "src/ast/type/texture_type.h"
 #include "src/ast/type/type.h"
 #include "src/ast/type/u32_type.h"
 #include "src/ast/type/vector_type.h"
@@ -3630,7 +3632,6 @@ bool FunctionEmitter::EmitSampledImageAccess(
       parser_impl_.GetMemoryObjectDeclarationForHandle(sampled_image_id, false);
   const auto* image =
       parser_impl_.GetMemoryObjectDeclarationForHandle(sampled_image_id, true);
-
   if (!sampler) {
     return Fail() << "interal error: couldn't find sampler for "
                   << inst.PrettyPrint();
@@ -3646,12 +3647,18 @@ bool FunctionEmitter::EmitSampledImageAccess(
   params.push_back(
       create<ast::IdentifierExpression>(namer_.Name(sampler->result_id())));
 
-  // Push the coordinates operand.
+  // Push the coordinates operands.
   // TODO(dneto): For explicit-Lod variations, we may have to convert from
   // integral coordinates to floating point coordinates.
-  // TODO(dneto): For arrayed access, split off the array layer.
+  // In WGSL, integer (unnormalized) coordinates are only used for texture
-  params.push_back(MakeOperand(inst, 1).expr);
+  // fetch (textureLoad on sampled image) or textureLoad or textureStore
-  uint32_t arg_index = 2;
+  // on storage images.
+  auto coords = MakeCoordinateOperandsForImageAccess(inst);
+  if (coords.empty()) {
+    return false;
+  }
+  params.insert(params.end(), coords.begin(), coords.end());
+  uint32_t arg_index = 2;  // Skip over texture and coordinate params
   const auto num_args = inst.NumInOperands();
 
   std::string builtin_name;
@@ -3725,6 +3732,137 @@ bool FunctionEmitter::EmitSampledImageAccess(
   return EmitConstDefOrWriteToHoistedVar(inst, {result_type, call_expr});
 }
 
+ast::ExpressionList FunctionEmitter::MakeCoordinateOperandsForImageAccess(
+    const spvtools::opt::Instruction& inst) {
+  if (!parser_impl_.success()) {
+    Fail();
+    return {};
+  }
+  if (inst.NumInOperands() == 0) {
+    Fail() << "internal error: not an image access instruction: "
+           << inst.PrettyPrint();
+    return {};
+  }
+  const auto sampled_image_id = inst.GetSingleWordInOperand(0);
+  const auto* image =
+      parser_impl_.GetMemoryObjectDeclarationForHandle(sampled_image_id, true);
+  if (!image) {
+    Fail() << "internal error: couldn't find image for " << inst.PrettyPrint();
+    return {};
+  }
+  if (image->NumInOperands() < 1) {
+    Fail() << "image access is missing a coordinate parameter: "
+           << inst.PrettyPrint();
+    return {};
+  }
+
+  // The coordinates parameter is always in position 1.
+  TypedExpression raw_coords(MakeOperand(inst, 1));
+  if (!raw_coords.type) {
+    return {};
+  }
+  ast::type::PointerType* type = parser_impl_.GetTypeForHandleVar(*image);
+  if (!parser_impl_.success()) {
+    Fail();
+    return {};
+  }
+  if (!type || !type->type()->IsTexture()) {
+    Fail() << "invalid texture type for " << image->PrettyPrint();
+    return {};
+  }
+  ast::type::TextureDimension dim = type->type()->AsTexture()->dim();
+  // Number of regular coordinates.
+  uint32_t num_axes = 0;
+  bool is_arrayed = false;
+  switch (dim) {
+    case ast::type::TextureDimension::k1d:
+      num_axes = 1;
+      break;
+    case ast::type::TextureDimension::k1dArray:
+      num_axes = 1;
+      is_arrayed = true;
+      break;
+    case ast::type::TextureDimension::k2d:
+      num_axes = 2;
+      break;
+    case ast::type::TextureDimension::k2dArray:
+      num_axes = 2;
+      is_arrayed = true;
+      break;
+    case ast::type::TextureDimension::k3d:
+      num_axes = 3;
+      break;
+    case ast::type::TextureDimension::kCube:
+      // For cubes, 3 coordinates form a direction vector.
+      num_axes = 3;
+      break;
+    case ast::type::TextureDimension::kCubeArray:
+      // For cubes, 3 coordinates form a direction vector.
+      num_axes = 3;
+      is_arrayed = true;
+      break;
+    default:
+      Fail() << "unsupported image dimensionality for " << type->type_name()
+             << " prompted by " << inst.PrettyPrint();
+      return {};
+  }
+  assert(num_axes <= 3);
+  const auto num_coords_required = num_axes + (is_arrayed ? 1 : 0);
+  uint32_t num_coords_supplied = 0;
+  if (raw_coords.type->IsF32()) {
+    num_coords_supplied = 1;
+  } else if (raw_coords.type->IsVector()) {
+    num_coords_supplied = raw_coords.type->AsVector()->size();
+  }
+  if (num_coords_supplied == 0) {
+    Fail() << "bad or unsupported coordinate type for image access: "
+           << inst.PrettyPrint();
+    return {};
+  }
+  if (num_coords_required > num_coords_supplied) {
+    Fail() << "image access required " << num_coords_required
+           << " coordinate components, but only " << num_coords_supplied
+           << " provided, in: " << inst.PrettyPrint();
+    return {};
+  }
+
+  auto prefix_swizzle = [this](uint32_t i) {
+    const char* prefix_name[] = {"", "x", "xy", "xyz"};
+    return ast_module_.create<ast::IdentifierExpression>(prefix_name[i & 3]);
+  };
+
+  ast::ExpressionList result;
+
+  // TODO(dneto): Convert component type if needed.
+  if (is_arrayed) {
+    // The source must be a vector, because it has enough components and has an
+    // array component. Use a vector swizzle to get the first `num_axes`
+    // components.
+    result.push_back(ast_module_.create<ast::MemberAccessorExpression>(
+        raw_coords.expr, prefix_swizzle(num_axes)));
+
+    // Now get the array index.
+    ast::Expression* array_index =
+        ast_module_.create<ast::MemberAccessorExpression>(raw_coords.expr,
+                                                          Swizzle(num_axes));
+    // Convert it to an unsigned integer type.
+    result.push_back(ast_module_.create<ast::TypeConstructorExpression>(
+        ast_module_.create<ast::type::U32Type>(),
+        ast::ExpressionList{array_index}));
+  } else {
+    if (num_coords_supplied == num_coords_required) {
+      // Pass the value through.
+      result.push_back(std::move(raw_coords.expr));
+    } else {
+      // There are more coordinates supplied than needed. So the source type is
+      // a vector. Use a vector swizzle to get the first `num_axes` components.
+      result.push_back(ast_module_.create<ast::MemberAccessorExpression>(
+          raw_coords.expr, prefix_swizzle(num_axes)));
+    }
+  }
+  return result;
+}
+
 }  // namespace spirv
 }  // namespace reader
 }  // namespace tint

src/reader/spirv/function.h

@@ -679,6 +679,20 @@ class FunctionEmitter {
   /// @returns the identifier expression for the @p i'th component
   ast::IdentifierExpression* Swizzle(uint32_t i);
 
+  /// Converts SPIR-V image coordinates from an image access instruction
+  /// (e.g. OpImageSampledImplicitLod) into an expression list consisting of
+  /// the texture coordinates, and an integral array index if the texture is
+  /// arrayed. The texture coordinate is a scalar for 1D textures, a vector of
+  /// 2 elements for a 2D texture, and a vector of 3 elements for a 3D or
+  /// Cube texture. Excess components are ignored, e.g. if the SPIR-V
+  /// coordinate is a 4-element vector but the image is a 2D non-arrayed
+  /// texture then the 3rd and 4th components are ignored.
+  /// On failure, issues an error and returns an empty expression list.
+  /// @param image_access the image access instruction
+  /// @returns an ExpressionList of the coordinate and array index (if any)
+  ast::ExpressionList MakeCoordinateOperandsForImageAccess(
+      const spvtools::opt::Instruction& image_access);
+
  private:
   /// @returns the store type for the OpVariable instruction, or
   /// null on failure.

src/reader/spirv/parser_impl.cc

@@ -1600,8 +1600,16 @@ ParserImpl::GetMemoryObjectDeclarationForHandle(uint32_t id,
   }
 }
 
-ast::type::Type* ParserImpl::GetTypeForHandleVar(
+ast::type::PointerType* ParserImpl::GetTypeForHandleVar(
     const spvtools::opt::Instruction& var) {
+  if (!success()) {
+    return nullptr;
+  }
+  auto where = handle_type_.find(&var);
+  if (where != handle_type_.end()) {
+    return where->second;
+  }
+
   // The WGSL handle type is determined by looking at information from
   // several sources:
   //    - the usage of the handle by image access instructions
@@ -1637,7 +1645,7 @@ ast::type::Type* ParserImpl::GetTypeForHandleVar(
           << var.PrettyPrint();
       return nullptr;
     default:
-      Fail() << "invalid type for image or sampler variable "
+      Fail() << "invalid type for image or sampler variable: "
              << var.PrettyPrint();
       return nullptr;
   }
@@ -1774,8 +1782,11 @@ ast::type::Type* ParserImpl::GetTypeForHandleVar(
   }
 
   // Form the pointer type.
-  return ast_module_.create<ast::type::PointerType>(
+  auto* result = ast_module_.create<ast::type::PointerType>(
       ast_store_type, ast::StorageClass::kUniformConstant);
+  // Remember it for later.
+  handle_type_[&var] = result;
+  return result;
 }
 
 bool ParserImpl::RegisterHandleUsage() {
@@ -1931,6 +1942,11 @@ Usage ParserImpl::GetHandleUsage(uint32_t id) const {
   return Usage();
 }
 
+const spvtools::opt::Instruction* ParserImpl::GetInstructionForTest(
+    uint32_t id) const {
+  return def_use_mgr_ ? def_use_mgr_->GetDef(id) : nullptr;
+}
+
 }  // namespace spirv
 }  // namespace reader
 }  // namespace tint

src/reader/spirv/parser_impl.h

@@ -115,6 +115,9 @@ class ParserImpl : Reader {
     return fail_stream_;
   }
 
+  /// @return true if failure has not yet occurred
+  bool success() const { return success_; }
+
   /// @returns the accumulated error string
   const std::string error() { return errors_.str(); }
 
@@ -383,7 +386,7 @@ class ParserImpl : Reader {
   /// @param id the SPIR-V result id.
   /// @return the Source record, or a default one
   Source GetSourceForResultIdForTest(uint32_t id) const;
-  /// Returns the soruce record for the given instruction.
+  /// Returns the source record for the given instruction.
   /// @param inst the SPIR-V instruction
   /// @return the Source record, or a default one
   Source GetSourceForInst(const spvtools::opt::Instruction* inst) const;
@@ -428,7 +431,13 @@ class ParserImpl : Reader {
   /// @param var the OpVariable instruction
   /// @returns the Tint AST type for the poiner-to-{sampler|texture} or null on
   /// error
-  ast::type::Type* GetTypeForHandleVar(const spvtools::opt::Instruction& var);
+  ast::type::PointerType* GetTypeForHandleVar(
+      const spvtools::opt::Instruction& var);
+
+  /// Returns the SPIR-V instruction with the given ID, or nullptr.
+  /// @param id the SPIR-V result ID
+  /// @returns the instruction, or nullptr on error
+  const spvtools::opt::Instruction* GetInstructionForTest(uint32_t id) const;
 
  private:
   /// Converts a specific SPIR-V type to a Tint type. Integer case
@@ -580,6 +589,9 @@ class ParserImpl : Reader {
   // Maps a memory-object-declaration instruction to any sampler or texture
   // usages implied by usages of the memory-object-declaration.
   std::unordered_map<const spvtools::opt::Instruction*, Usage> handle_usage_;
+  // The inferred pointer type for the given handle variable.
+  std::unordered_map<const spvtools::opt::Instruction*, ast::type::PointerType*>
+      handle_type_;
 };
 
 }  // namespace spirv

src/reader/spirv/parser_impl_handle_test.cc

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include <string>
+#include <vector>
 
 #include "gmock/gmock.h"
 #include "src/reader/spirv/function.h"
@@ -39,7 +40,7 @@ std::string Preamble() {
   )";
 }
 
-std::string CommonTypes() {
+std::string CommonBasicTypes() {
   return R"(
     %void = OpTypeVoid
     %voidfn = OpTypeFunction %void
@@ -50,6 +51,7 @@ std::string CommonTypes() {
 
     %int_3 = OpConstant %uint 3
     %int_4 = OpConstant %uint 4
+    %uint_0 = OpConstant %uint 0
     %uint_1 = OpConstant %uint 1
     %uint_2 = OpConstant %uint 2
     %uint_100 = OpConstant %uint 100
@@ -63,13 +65,26 @@ std::string CommonTypes() {
     %v4float = OpTypeVector %float 4
 
     %float_null = OpConstantNull %float
+    %float_1 = OpConstant %float 1
+    %float_2 = OpConstant %float 2
+    %float_3 = OpConstant %float 3
+    %float_4 = OpConstant %float 4
     %float_7 = OpConstant %float 7
     %v2float_null = OpConstantNull %v2float
     %v3float_null = OpConstantNull %v3float
     %v4float_null = OpConstantNull %v4float
 
+    %vf12 = OpConstantComposite %v2float %float_1 %float_2
+    %vf123 = OpConstantComposite %v3float %float_1 %float_2 %float_3
+    %vf1234 = OpConstantComposite %v4float %float_1 %float_2 %float_3 %float_4
+
     %depth = OpConstant %float 0.2
     %offsets2d = OpConstantComposite %v2int %int_3 %int_4
+  )";
+}
+
+std::string CommonImageTypes() {
+  return R"(
 
 ; Define types for all sampler and texture types that can map to WGSL,
 ; modulo texel formats for storage textures. For now, we limit
@@ -194,6 +209,10 @@ std::string CommonTypes() {
   )";
 }
 
+std::string CommonTypes() {
+  return CommonBasicTypes() + CommonImageTypes();
+}
+
 TEST_F(SpvParserTest,
        GetMemoryObjectDeclarationForHandle_WellFormedButNotAHandle) {
   const auto assembly = Preamble() + CommonTypes() + R"(
@@ -1801,6 +1820,403 @@ INSTANTIATE_TEST_SUITE_P(
             )
           })"}));
 
+struct ImageCoordsCase {
+  std::string spirv_image_type_details;  // SPIR-V image type, excluding result
+                                         // ID and opcode
+  std::string spirv_image_access;
+  std::string expected_error;
+  std::vector<std::string> expected_expressions;
+};
+
+inline std::ostream& operator<<(std::ostream& out, const ImageCoordsCase& c) {
+  out << "ImageAccessCase(" << c.spirv_image_type_details << "\n"
+      << c.spirv_image_access << "\n";
+
+  for (auto e : c.expected_expressions) {
+    out << e << ",";
+  }
+  out << ")" << std::endl;
+  return out;
+}
+
+using SpvParserTest_ImageCoordsTest =
+    SpvParserTestBase<::testing::TestWithParam<ImageCoordsCase>>;
+
+TEST_P(SpvParserTest_ImageCoordsTest, MakeCoordinateOperandsForImageAccess) {
+  const auto assembly = Preamble() + R"(
+     OpEntryPoint Fragment %100 "main"
+     OpExecutionMode %100 OriginUpperLeft
+     OpName %float_var "float_var"
+     OpName %ptr_float "ptr_float"
+     OpDecorate %10 DescriptorSet 0
+     OpDecorate %10 Binding 0
+     OpDecorate %20 DescriptorSet 2
+     OpDecorate %20 Binding 1
+     OpDecorate %30 DescriptorSet 0
+     OpDecorate %30 Binding 1
+)" + CommonBasicTypes() +
+                        R"(
+     %sampler = OpTypeSampler
+     %ptr_sampler = OpTypePointer UniformConstant %sampler
+     %im_ty = OpTypeImage )" +
+                        GetParam().spirv_image_type_details + R"(
+     %ptr_im_ty = OpTypePointer UniformConstant %im_ty
+
+     %si_ty = OpTypeSampledImage %im_ty
+     %coords = OpConstantNull %v2float
+
+     %ptr_float = OpTypePointer Function %float
+
+     %10 = OpVariable %ptr_sampler UniformConstant
+     %20 = OpVariable %ptr_im_ty UniformConstant
+     %30 = OpVariable %ptr_sampler UniformConstant ; comparison sampler, when needed
+
+     %100 = OpFunction %void None %voidfn
+     %entry = OpLabel
+
+     %float_var = OpVariable %ptr_float Function
+
+     ; create some names that will become WGSL variables x_1, x_12, and so on.
+     %1 = OpCopyObject %float %float_1
+     %12 = OpCopyObject %v2float %vf12
+     %123 = OpCopyObject %v3float %vf123
+     %1234 = OpCopyObject %v4float %vf1234
+
+
+     %sam = OpLoad %sampler %10
+     %im = OpLoad %im_ty %20
+     %sampled_image = OpSampledImage %si_ty %im %sam
+
+)" + GetParam().spirv_image_access +
+                        R"(
+     ; Use an anchor for the cases when the image access doesn't have a result ID.
+     %1000 = OpCopyObject %uint %uint_0
+
+     OpReturn
+     OpFunctionEnd
+  )";
+  auto p = parser(test::Assemble(assembly));
+  if (!p->BuildAndParseInternalModule()) {
+    EXPECT_THAT(p->error(), Eq(GetParam().expected_error));
+  } else {
+    EXPECT_TRUE(p->error().empty()) << p->error();
+    FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100));
+    // We actually have to generate the module to cache expressions for the
+    // result IDs, particularly the OpCopyObject
+    fe.Emit();
+
+    const spvtools::opt::Instruction* anchor = p->GetInstructionForTest(1000);
+    ASSERT_NE(anchor, nullptr);
+    const spvtools::opt::Instruction& image_access = *(anchor->PreviousNode());
+
+    ast::ExpressionList result =
+        fe.MakeCoordinateOperandsForImageAccess(image_access);
+    if (GetParam().expected_error.empty()) {
+      EXPECT_TRUE(fe.success()) << p->error();
+      EXPECT_TRUE(p->error().empty());
+      std::vector<std::string> result_strings;
+      for (auto* expr : result) {
+        ASSERT_NE(expr, nullptr);
+        result_strings.push_back(expr->str());
+      }
+      EXPECT_THAT(result_strings,
+                  ::testing::ContainerEq(GetParam().expected_expressions));
+    } else {
+      EXPECT_FALSE(fe.success());
+      EXPECT_THAT(p->error(), Eq(GetParam().expected_error));
+      EXPECT_TRUE(result.empty());
+    }
+  }
+}
+
+INSTANTIATE_TEST_SUITE_P(Good_1D,
+                         SpvParserTest_ImageCoordsTest,
+                         ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+                             {"%float 1D 0 0 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %1",
+                              "",
+                              {"Identifier[not set]{x_1}\n"}},
+                             {"%float 1D 0 0 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %12",  // one excess arg
+                              "",
+                              {R"(MemberAccessor[not set]{
+  Identifier[not set]{x_12}
+  Identifier[not set]{x}
+}
+)"}},
+                             {"%float 1D 0 0 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %123",  // two excess args
+                              "",
+                              {R"(MemberAccessor[not set]{
+  Identifier[not set]{x_123}
+  Identifier[not set]{x}
+}
+)"}},
+                             {"%float 1D 0 0 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %1234",  // three excess args
+                              "",
+                              {R"(MemberAccessor[not set]{
+  Identifier[not set]{x_1234}
+  Identifier[not set]{x}
+}
+)"}}}));
+
+INSTANTIATE_TEST_SUITE_P(Good_1DArray,
+                         SpvParserTest_ImageCoordsTest,
+                         ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+                             {"%float 1D 0 1 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %12",
+                              "",
+                              {
+                                  R"(MemberAccessor[not set]{
+  Identifier[not set]{x_12}
+  Identifier[not set]{x}
+}
+)",
+                                  R"(TypeConstructor[not set]{
+  __u32
+  MemberAccessor[not set]{
+    Identifier[not set]{x_12}
+    Identifier[not set]{y}
+  }
+}
+)"}},
+                             {"%float 1D 0 1 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %123",  // one excess arg
+                              "",
+                              {
+                                  R"(MemberAccessor[not set]{
+  Identifier[not set]{x_123}
+  Identifier[not set]{x}
+}
+)",
+                                  R"(TypeConstructor[not set]{
+  __u32
+  MemberAccessor[not set]{
+    Identifier[not set]{x_123}
+    Identifier[not set]{y}
+  }
+}
+)"}},
+                             {"%float 1D 0 1 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %1234",  // two excess args
+                              "",
+                              {
+                                  R"(MemberAccessor[not set]{
+  Identifier[not set]{x_1234}
+  Identifier[not set]{x}
+}
+)",
+                                  R"(TypeConstructor[not set]{
+  __u32
+  MemberAccessor[not set]{
+    Identifier[not set]{x_1234}
+    Identifier[not set]{y}
+  }
+}
+)"}}}));
+
+INSTANTIATE_TEST_SUITE_P(Good_2D,
+                         SpvParserTest_ImageCoordsTest,
+                         ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+                             {"%float 2D 0 0 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %12",
+                              "",
+                              {"Identifier[not set]{x_12}\n"}},
+                             {"%float 2D 0 0 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %123",  // one excess arg
+                              "",
+                              {R"(MemberAccessor[not set]{
+  Identifier[not set]{x_123}
+  Identifier[not set]{xy}
+}
+)"}},
+                             {"%float 2D 0 0 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %1234",  // two excess args
+                              "",
+                              {R"(MemberAccessor[not set]{
+  Identifier[not set]{x_1234}
+  Identifier[not set]{xy}
+}
+)"}}}));
+
+INSTANTIATE_TEST_SUITE_P(Good_2DArray,
+                         SpvParserTest_ImageCoordsTest,
+                         ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+                             {"%float 2D 0 1 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %123",
+                              "",
+                              {
+                                  R"(MemberAccessor[not set]{
+  Identifier[not set]{x_123}
+  Identifier[not set]{xy}
+}
+)",
+                                  R"(TypeConstructor[not set]{
+  __u32
+  MemberAccessor[not set]{
+    Identifier[not set]{x_123}
+    Identifier[not set]{z}
+  }
+}
+)"}},
+                             {"%float 2D 0 1 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod %v4float "
+                              "%sampled_image %1234",  // one excess arg
+                              "",
+                              {
+                                  R"(MemberAccessor[not set]{
+  Identifier[not set]{x_1234}
+  Identifier[not set]{xy}
+}
+)",
+                                  R"(TypeConstructor[not set]{
+  __u32
+  MemberAccessor[not set]{
+    Identifier[not set]{x_1234}
+    Identifier[not set]{z}
+  }
+}
+)"}}}));
+
+INSTANTIATE_TEST_SUITE_P(Good_3D,
+                         SpvParserTest_ImageCoordsTest,
+                         ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+                             {"%float 3D 0 0 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod "
+                              "%v4float "
+                              "%sampled_image %123",
+                              "",
+                              {"Identifier[not set]{x_123}\n"}},
+                             {"%float 3D 0 0 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod "
+                              "%v4float "
+                              "%sampled_image %1234",  // one excess
+                                                       // arg
+                              "",
+                              {R"(MemberAccessor[not set]{
+  Identifier[not set]{x_1234}
+  Identifier[not set]{xyz}
+}
+)"}}}));
+
+INSTANTIATE_TEST_SUITE_P(Good_Cube,
+                         SpvParserTest_ImageCoordsTest,
+                         ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+                             {"%float Cube 0 0 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod "
+                              "%v4float "
+                              "%sampled_image %123",
+                              "",
+                              {"Identifier[not set]{x_123}\n"}},
+                             {"%float Cube 0 0 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod "
+                              "%v4float "
+                              "%sampled_image %1234",  // one excess
+                                                       // arg
+                              "",
+                              {R"(MemberAccessor[not set]{
+  Identifier[not set]{x_1234}
+  Identifier[not set]{xyz}
+}
+)"}}}));
+
+INSTANTIATE_TEST_SUITE_P(Good_CubeArray,
+                         SpvParserTest_ImageCoordsTest,
+                         ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+                             {"%float Cube 0 1 0 1 Unknown",
+                              "%result = OpImageSampleImplicitLod "
+                              "%v4float "
+                              "%sampled_image %1234",
+                              "",
+                              {R"(MemberAccessor[not set]{
+  Identifier[not set]{x_1234}
+  Identifier[not set]{xyz}
+}
+)",
+                               R"(TypeConstructor[not set]{
+  __u32
+  MemberAccessor[not set]{
+    Identifier[not set]{x_1234}
+    Identifier[not set]{w}
+  }
+}
+)"}}}));
+
+INSTANTIATE_TEST_SUITE_P(BadInstructions,
+                         SpvParserTest_ImageCoordsTest,
+                         ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+                             {"%float 1D 0 0 0 1 Unknown",
+                              "OpNop",
+                              "internal error: not an image access "
+                              "instruction: OpNop",
+                              {}},
+                             {"%float 1D 0 0 0 1 Unknown",
+                              "%foo = OpCopyObject %float %float_1",
+                              "internal error: couldn't find image for "
+                              "%50 = OpCopyObject %6 %26",
+                              {}},
+                             {"%float 1D 0 0 0 1 Unknown",
+                              "OpStore %float_var %float_1",
+                              "invalid type for image or sampler "
+                              "variable: %2 = OpVariable %3 Function",
+                              {}},
+                             // An example with a missing coordinate
+                             // won't assemble, so we skip it.
+                         }));
+
+INSTANTIATE_TEST_SUITE_P(
+    Bad_Coordinate,
+    SpvParserTest_ImageCoordsTest,
+    ::testing::ValuesIn(std::vector<ImageCoordsCase>{
+        {"%float 2D 0 0 0 1 Unknown",
+         "%result = OpImageSampleImplicitLod "
+         // bad type for coordinate: not a number
+         "%v4float %sampled_image %float_var",
+         "bad or unsupported coordinate type for image access: %50 = "
+         "OpImageSampleImplicitLod %24 %49 %2",
+         {}},
+        {"%float 1D 0 1 0 1 Unknown",  // 1DArray
+         "%result = OpImageSampleImplicitLod "
+         // 1 component, but need 2
+         "%v4float %sampled_image %1",
+         "image access required 2 coordinate components, but only 1 provided, "
+         "in: %50 = OpImageSampleImplicitLod %24 %49 %1",
+         {}},
+        {"%float 2D 0 0 0 1 Unknown",  // 2D
+         "%result = OpImageSampleImplicitLod "
+         // 1 component, but need 2
+         "%v4float %sampled_image %1",
+         "image access required 2 coordinate components, but only 1 provided, "
+         "in: %50 = OpImageSampleImplicitLod %24 %49 %1",
+         {}},
+        {"%float 2D 0 1 0 1 Unknown",  // 2DArray
+         "%result = OpImageSampleImplicitLod "
+         // 2 component, but need 3
+         "%v4float %sampled_image %12",
+         "image access required 3 coordinate components, but only 2 provided, "
+         "in: %50 = OpImageSampleImplicitLod %24 %49 %12",
+         {}},
+        {"%float 3D 0 0 0 1 Unknown",  // 3D
+         "%result = OpImageSampleImplicitLod "
+         // 2 components, but need 3
+         "%v4float %sampled_image %12",
+         "image access required 3 coordinate components, but only 2 provided, "
+         "in: %50 = OpImageSampleImplicitLod %24 %49 %12",
+         {}},
+    }));
+
 }  // namespace
 }  // namespace spirv
 }  // namespace reader