spirv-reader: Infer a handle type when needed

Fixed: tint:374
Change-Id: I4785a48d456a6b5ba1fa8c9950b4c72d00853fc9
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/33981
Commit-Queue: David Neto <dneto@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Auto-Submit: David Neto <dneto@google.com>

src/reader/spirv/parser_impl.cc

@@ -1604,96 +1604,76 @@ ParserImpl::GetMemoryObjectDeclarationForHandle(uint32_t id,
 
 ast::type::Type* ParserImpl::GetTypeForHandleVar(
     const spvtools::opt::Instruction& var) {
-  // This can be a sampler or image.
+  // The WGSL handle type is determined by looking at information from
-  // Determine it from the usage inferred for the variable.
+  // several sources:
-  const Usage& usage = handle_usage_[&var];
+  //    - the usage of the handle by image access instructions
+  //    - the SPIR-V type declaration
+  // Each source does not have enough information to completely determine
+  // the result.
+
+  // Messages are phrased in terms of images and samplers because those
+  // are the only SPIR-V handles supported by WGSL.
+
+  // Get the SPIR-V handle type.
+  const auto* ptr_type = def_use_mgr_->GetDef(var.type_id());
+  if (!ptr_type) {
+    Fail() << "Invalid type for variable " << var.PrettyPrint();
+    return nullptr;
+  }
+  const auto* raw_handle_type =
+      def_use_mgr_->GetDef(ptr_type->GetSingleWordInOperand(1));
+  if (!raw_handle_type) {
+    Fail() << "Invalid pointer type for variable " << var.PrettyPrint();
+    return nullptr;
+  }
+  switch (raw_handle_type->opcode()) {
+    case SpvOpTypeSampler:
+    case SpvOpTypeImage:
+      // The expected cases.
+      break;
+    case SpvOpTypeArray:
+    case SpvOpTypeRuntimeArray:
+      Fail()
+          << "arrays of textures or samplers are not supported in WGSL; can't "
+             "translate variable "
+          << var.PrettyPrint();
+      return nullptr;
+    default:
+      Fail() << "invalid type for image or sampler variable "
+             << var.PrettyPrint();
+      return nullptr;
+  }
+
+  // The variable could be a sampler or image.
+  // Where possible, determine which one it is from the usage inferred
+  // for the variable.
+  Usage usage = handle_usage_[&var];
   if (!usage.IsValid()) {
     Fail() << "Invalid sampler or texture usage for variable "
            << var.PrettyPrint() << "\n"
            << usage;
     return nullptr;
   }
+  // Infer a handle type, if usage didn't already tell us.
   if (!usage.IsComplete()) {
-    // TODO(dneto): In SPIR-V you could statically reference a texture or
+    // In SPIR-V you could statically reference a texture or sampler without
-    // sampler without using it in a way that gives us a clue on how to declare
+    // using it in a way that gives us a clue on how to declare it.  Look inside
-    // it.  In that case look at the underlying OpTypeSampler or OpTypeImage; in
+    // the store type to infer a usage.
-    // the OpTypeImage see if it has a format.  Sampled images alway have
+    if (raw_handle_type->opcode() == SpvOpTypeSampler) {
-    // Unknown format.  For WGSL, storage images always have a format.  And then
+      usage.AddSampler();
-    // check for NonWritable or NonReadabl
-    Fail() << "Unsupported: Incomplete usage on samper or texture usage for "
-              "variable "
-           << var.PrettyPrint() << "\n"
-           << usage;
-    return nullptr;
-  }
-  ast::type::Type* ast_store_type = nullptr;
-  if (usage.IsSampler()) {
-    ast_store_type = ast_module_.create<ast::type::SamplerType>(
-        usage.IsComparisonSampler() ? ast::type::SamplerKind::kComparisonSampler
-                                    : ast::type::SamplerKind::kSampler);
-  } else if (usage.IsTexture()) {
-    const auto* ptr_type = def_use_mgr_->GetDef(var.type_id());
-    if (!ptr_type) {
-      Fail() << "Invalid type for variable " << var.PrettyPrint();
-      return nullptr;
-    }
-    const auto* raw_image_type =
-        def_use_mgr_->GetDef(ptr_type->GetSingleWordInOperand(1));
-    if (!raw_image_type) {
-      Fail() << "Invalid pointer type for variable " << var.PrettyPrint();
-      return nullptr;
-    }
-    switch (raw_image_type->opcode()) {
-      case SpvOpTypeImage:  // The expected case.
-        break;
-      case SpvOpTypeArray:
-      case SpvOpTypeRuntimeArray:
-        Fail() << "arrays of textures are not supported in WGSL; can't "
-                  "translate variable "
-               << var.PrettyPrint();
-        return nullptr;
-      default:
-        Fail() << "invalid type for image variable " << var.PrettyPrint();
-        return nullptr;
-    }
-    const spvtools::opt::analysis::Image* image_type =
-        type_mgr_->GetType(raw_image_type->result_id())->AsImage();
-    if (!image_type) {
-      Fail() << "internal error: Couldn't look up image type"
-             << raw_image_type->PrettyPrint();
-      return nullptr;
-    }
-
-    const ast::type::TextureDimension dim =
-        enum_converter_.ToDim(image_type->dim(), image_type->is_arrayed());
-    if (dim == ast::type::TextureDimension::kNone) {
-      return nullptr;
-    }
-
-    // WGSL textures are always formatted.  Unformatted textures are always
-    // sampled.
-    if (usage.IsSampledTexture() ||
-        (image_type->format() == SpvImageFormatUnknown)) {
-      // Make a sampled texture type.
-      auto* ast_sampled_component_type =
-          ConvertType(raw_image_type->GetSingleWordInOperand(0));
-
-      // Vulkan ignores the depth parameter on OpImage, so pay attention to the
-      // usage as well.  That is, it's valid for a Vulkan shader to use an
-      // OpImage variable with an OpImage*Dref* instruction.  In WGSL we must
-      // treat that as a depth texture.
-      if (image_type->depth() || usage.IsDepthTexture()) {
-        ast_store_type = ast_module_.create<ast::type::DepthTextureType>(dim);
-      } else if (image_type->is_multisampled()) {
-        // Multisampled textures are never depth textures.
-        ast_store_type = ast_module_.create<ast::type::MultisampledTextureType>(
-            dim, ast_sampled_component_type);
     } else {
-        ast_store_type = ast_module_.create<ast::type::SampledTextureType>(
+      // It's a texture.
-            dim, ast_sampled_component_type);
+      if (raw_handle_type->NumInOperands() != 7) {
+        Fail() << "invalid SPIR-V image type: expected 7 operands: "
+               << raw_handle_type->PrettyPrint();
+        return nullptr;
       }
-    } else {
+      const auto sampled_param = raw_handle_type->GetSingleWordInOperand(5);
-      // Make a storage texture.
+      const auto format_param = raw_handle_type->GetSingleWordInOperand(6);
+      // Only storage images have a format.
+      if ((format_param != SpvImageFormatUnknown) ||
+          sampled_param == 2 /* without sampler */) {
+        // Get NonWritable and NonReadable attributes of the variable.
         bool is_nonwritable = false;
         bool is_nonreadable = false;
         for (const auto& deco : GetDecorationsFor(var.result_id())) {
@@ -1716,7 +1696,70 @@ ast::type::Type* ParserImpl::GetTypeForHandleVar(
               << "storage image variable is neither NonWritable nor NonReadable"
               << var.PrettyPrint();
         }
-      const auto access = is_nonwritable ? ast::AccessControl::kReadOnly
+        // Let's make it one of the storage textures.
+        if (is_nonwritable) {
+          usage.AddStorageReadTexture();
+        } else {
+          usage.AddStorageWriteTexture();
+        }
+      } else {
+        usage.AddSampledTexture();
+      }
+    }
+    if (!usage.IsComplete()) {
+      Fail()
+          << "internal error: should have inferred a complete handle type. got "
+          << usage.to_str();
+      return nullptr;
+    }
+  }
+
+  // Construct the Tint handle type.
+  ast::type::Type* ast_store_type = nullptr;
+  if (usage.IsSampler()) {
+    ast_store_type = ast_module_.create<ast::type::SamplerType>(
+        usage.IsComparisonSampler() ? ast::type::SamplerKind::kComparisonSampler
+                                    : ast::type::SamplerKind::kSampler);
+  } else if (usage.IsTexture()) {
+    const spvtools::opt::analysis::Image* image_type =
+        type_mgr_->GetType(raw_handle_type->result_id())->AsImage();
+    if (!image_type) {
+      Fail() << "internal error: Couldn't look up image type"
+             << raw_handle_type->PrettyPrint();
+      return nullptr;
+    }
+
+    const ast::type::TextureDimension dim =
+        enum_converter_.ToDim(image_type->dim(), image_type->is_arrayed());
+    if (dim == ast::type::TextureDimension::kNone) {
+      return nullptr;
+    }
+
+    // WGSL textures are always formatted.  Unformatted textures are always
+    // sampled.
+    if (usage.IsSampledTexture() ||
+        (image_type->format() == SpvImageFormatUnknown)) {
+      // Make a sampled texture type.
+      auto* ast_sampled_component_type =
+          ConvertType(raw_handle_type->GetSingleWordInOperand(0));
+
+      // Vulkan ignores the depth parameter on OpImage, so pay attention to the
+      // usage as well.  That is, it's valid for a Vulkan shader to use an
+      // OpImage variable with an OpImage*Dref* instruction.  In WGSL we must
+      // treat that as a depth texture.
+      if (image_type->depth() || usage.IsDepthTexture()) {
+        ast_store_type = ast_module_.create<ast::type::DepthTextureType>(dim);
+      } else if (image_type->is_multisampled()) {
+        // Multisampled textures are never depth textures.
+        ast_store_type = ast_module_.create<ast::type::MultisampledTextureType>(
+            dim, ast_sampled_component_type);
+      } else {
+        ast_store_type = ast_module_.create<ast::type::SampledTextureType>(
+            dim, ast_sampled_component_type);
+      }
+    } else {
+      const auto access = usage.IsStorageReadTexture()
+                              ? ast::AccessControl::kReadOnly
                               : ast::AccessControl::kWriteOnly;
       const auto format = enum_converter_.ToImageFormat(image_type->format());
       if (format == ast::type::ImageFormat::kNone) {
@@ -1731,6 +1774,7 @@ ast::type::Type* ParserImpl::GetTypeForHandleVar(
            << var.PrettyPrint();
     return nullptr;
   }
+
   // Form the pointer type.
   return ast_module_.create<ast::type::PointerType>(
       ast_store_type, ast::StorageClass::kUniformConstant);

src/reader/spirv/parser_impl.h

@@ -44,6 +44,23 @@
 #include "src/reader/spirv/usage.h"
 #include "src/source.h"
 
+/// This is the implementation of the SPIR-V parser for Tint.
+
+/// Notes on terminology:
+///
+/// A WGSL "handle" is an opaque object used for accessing a resource via
+/// special builtins.  In SPIR-V, a handle is stored a variable in the
+/// UniformConstant storage class.  The handles supported by SPIR-V are:
+///   - images, both sampled texture and storage image
+///   - samplers
+///   - combined image+sampler
+///   - acceleration structures for raytracing.
+///
+/// WGSL only supports samplers and images, but calls images "textures".
+/// When emitting errors, we aim to use terminology most likely to be
+/// familiar to Vulkan SPIR-V developers.  We will tend to use "image"
+/// and "sampler" instead of "handle".
+
 namespace tint {
 namespace reader {
 namespace spirv {

src/reader/spirv/parser_impl_handle_test.cc

@@ -1061,6 +1061,117 @@ INSTANTIATE_TEST_SUITE_P(
 
 // Test emission of handle variables.
 
+// Test emission of variables where we don't have enough clues from their
+// use in image access instructions in executable code.  For these we have
+// to infer usage from the SPIR-V sampler or image type.
+struct DeclUnderspecifiedHandleCase {
+  std::string decorations;  // SPIR-V decorations
+  std::string inst;         // SPIR-V variable declarations
+  std::string var_decl;     // WGSL variable declaration
+};
+inline std::ostream& operator<<(std::ostream& out,
+                                const DeclUnderspecifiedHandleCase& c) {
+  out << "DeclUnderspecifiedHandleCase(" << c.inst << "\n" << c.var_decl << ")";
+  return out;
+}
+
+using SpvParserTest_DeclUnderspecifiedHandle =
+    SpvParserTestBase<::testing::TestWithParam<DeclUnderspecifiedHandleCase>>;
+
+TEST_P(SpvParserTest_DeclUnderspecifiedHandle, Variable) {
+  const auto assembly = Preamble() + R"(
+     OpDecorate %10 DescriptorSet 0
+     OpDecorate %10 Binding 0
+)" + GetParam().decorations +
+                        CommonTypes() + GetParam().inst +
+                        R"(
+
+     %main = OpFunction %void None %voidfn
+     %entry = OpLabel
+     OpReturn
+     OpFunctionEnd
+  )";
+  auto p = parser(test::Assemble(assembly));
+  ASSERT_TRUE(p->BuildAndParseInternalModule()) << p->error() << assembly;
+  EXPECT_TRUE(p->error().empty()) << p->error();
+  const auto module = p->module().to_str();
+  EXPECT_THAT(module, HasSubstr(GetParam().var_decl)) << module;
+}
+
+INSTANTIATE_TEST_SUITE_P(Samplers,
+                         SpvParserTest_DeclUnderspecifiedHandle,
+                         ::testing::Values(
+
+                             DeclUnderspecifiedHandleCase{"", R"(
+         %ptr = OpTypePointer UniformConstant %sampler
+         %10 = OpVariable %ptr UniformConstant
+)",
+                                                          R"(
+  DecoratedVariable{
+    Decorations{
+      SetDecoration{0}
+      BindingDecoration{0}
+    }
+    x_10
+    uniform_constant
+    __sampler_sampler
+  })"}));
+
+INSTANTIATE_TEST_SUITE_P(Images,
+                         SpvParserTest_DeclUnderspecifiedHandle,
+                         ::testing::Values(
+
+                             DeclUnderspecifiedHandleCase{"", R"(
+         %10 = OpVariable %ptr_f_texture_1d UniformConstant
+)",
+                                                          R"(
+  DecoratedVariable{
+    Decorations{
+      SetDecoration{0}
+      BindingDecoration{0}
+    }
+    x_10
+    uniform_constant
+    __sampled_texture_1d__f32
+  })"},
+                             DeclUnderspecifiedHandleCase{R"(
+         OpDecorate %10 NonWritable
+)",
+                                                          R"(
+         %10 = OpVariable %ptr_f_storage_1d UniformConstant
+)",
+                                                          R"(
+  DecoratedVariable{
+    Decorations{
+      SetDecoration{0}
+      BindingDecoration{0}
+    }
+    x_10
+    uniform_constant
+    __storage_texture_read_only_1d_rg32float
+  })"},
+                             DeclUnderspecifiedHandleCase{R"(
+         OpDecorate %10 NonReadable
+)",
+                                                          R"(
+         %10 = OpVariable %ptr_f_storage_1d UniformConstant
+)",
+                                                          R"(
+  DecoratedVariable{
+    Decorations{
+      SetDecoration{0}
+      BindingDecoration{0}
+    }
+    x_10
+    uniform_constant
+    __storage_texture_write_only_1d_rg32float
+  })"}
+
+                             ));
+
+// Test emission of variables when we have sampled image accesses in
+// executable code.
+
 struct DeclSampledImageCase {
   std::string inst;             // The provoking image access instruction.
   std::string var_decl;         // WGSL variable declaration
@@ -1068,7 +1179,7 @@ struct DeclSampledImageCase {
 };
 inline std::ostream& operator<<(std::ostream& out,
                                 const DeclSampledImageCase& c) {
-  out << "UsageSampledImageCase(" << c.inst << "\n"
+  out << "DeclSampledImageCase(" << c.inst << "\n"
       << c.var_decl << "\n"
       << c.texture_builtin << ")";
   return out;