From ddaf59016be33468e3997180823d5a5a096c7445 Mon Sep 17 00:00:00 2001 From: David Neto Date: Thu, 26 Nov 2020 15:35:12 +0000 Subject: [PATCH] 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 Reviewed-by: Ben Clayton Auto-Submit: David Neto --- src/reader/spirv/parser_impl.cc | 176 ++++++++++++-------- src/reader/spirv/parser_impl.h | 17 ++ src/reader/spirv/parser_impl_handle_test.cc | 113 ++++++++++++- 3 files changed, 239 insertions(+), 67 deletions(-) diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc index f41c4ed473..d6ed08bbf7 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/src/reader/spirv/parser_impl.cc @@ -1604,63 +1604,128 @@ ParserImpl::GetMemoryObjectDeclarationForHandle(uint32_t id, ast::type::Type* ParserImpl::GetTypeForHandleVar( const spvtools::opt::Instruction& var) { - // This can be a sampler or image. - // Determine it from the usage inferred for the variable. - const Usage& usage = handle_usage_[&var]; + // The WGSL handle type is determined by looking at information from + // several sources: + // - 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 - // sampler without using it in a way that gives us a clue on how to declare - // it. In that case look at the underlying OpTypeSampler or OpTypeImage; in - // the OpTypeImage see if it has a format. Sampled images alway have - // Unknown format. For WGSL, storage images always have a format. And then - // check for NonWritable or NonReadabl - Fail() << "Unsupported: Incomplete usage on samper or texture usage for " - "variable " - << var.PrettyPrint() << "\n" - << usage; - return nullptr; + // In SPIR-V you could statically reference a texture or sampler without + // using it in a way that gives us a clue on how to declare it. Look inside + // the store type to infer a usage. + if (raw_handle_type->opcode() == SpvOpTypeSampler) { + usage.AddSampler(); + } else { + // It's a texture. + if (raw_handle_type->NumInOperands() != 7) { + Fail() << "invalid SPIR-V image type: expected 7 operands: " + << raw_handle_type->PrettyPrint(); + return nullptr; + } + const auto sampled_param = raw_handle_type->GetSingleWordInOperand(5); + 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())) { + if (deco.size() != 1) { + continue; + } + if (deco[0] == SpvDecorationNonWritable) { + is_nonwritable = true; + } + if (deco[0] == SpvDecorationNonReadable) { + is_nonreadable = true; + } + } + if (is_nonwritable && is_nonreadable) { + Fail() << "storage image variable is both NonWritable and NonReadable" + << var.PrettyPrint(); + } + if (!is_nonwritable && !is_nonreadable) { + Fail() + << "storage image variable is neither NonWritable nor NonReadable" + << var.PrettyPrint(); + } + // 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( 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(); + type_mgr_->GetType(raw_handle_type->result_id())->AsImage(); if (!image_type) { Fail() << "internal error: Couldn't look up image type" - << raw_image_type->PrettyPrint(); + << raw_handle_type->PrettyPrint(); return nullptr; } @@ -1676,7 +1741,7 @@ ast::type::Type* ParserImpl::GetTypeForHandleVar( (image_type->format() == SpvImageFormatUnknown)) { // Make a sampled texture type. auto* ast_sampled_component_type = - ConvertType(raw_image_type->GetSingleWordInOperand(0)); + 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 @@ -1693,31 +1758,9 @@ ast::type::Type* ParserImpl::GetTypeForHandleVar( dim, ast_sampled_component_type); } } else { - // Make a storage texture. - bool is_nonwritable = false; - bool is_nonreadable = false; - for (const auto& deco : GetDecorationsFor(var.result_id())) { - if (deco.size() != 1) { - continue; - } - if (deco[0] == SpvDecorationNonWritable) { - is_nonwritable = true; - } - if (deco[0] == SpvDecorationNonReadable) { - is_nonreadable = true; - } - } - if (is_nonwritable && is_nonreadable) { - Fail() << "storage image variable is both NonWritable and NonReadable" - << var.PrettyPrint(); - } - if (!is_nonwritable && !is_nonreadable) { - Fail() - << "storage image variable is neither NonWritable nor NonReadable" - << var.PrettyPrint(); - } - const auto access = is_nonwritable ? ast::AccessControl::kReadOnly - : ast::AccessControl::kWriteOnly; + 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) { return nullptr; @@ -1731,6 +1774,7 @@ ast::type::Type* ParserImpl::GetTypeForHandleVar( << var.PrettyPrint(); return nullptr; } + // Form the pointer type. return ast_module_.create( ast_store_type, ast::StorageClass::kUniformConstant); diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h index bfafb4b762..e25b138650 100644 --- a/src/reader/spirv/parser_impl.h +++ b/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 { diff --git a/src/reader/spirv/parser_impl_handle_test.cc b/src/reader/spirv/parser_impl_handle_test.cc index 06109be16a..79e14018f6 100644 --- a/src/reader/spirv/parser_impl_handle_test.cc +++ b/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>; + +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;