From 8144af91b4f901b5083b168c6600f4b0f3947d80 Mon Sep 17 00:00:00 2001 From: David Neto Date: Thu, 7 Jan 2021 01:38:41 +0000 Subject: [PATCH] spirv-reader: ignore PointSize builtin declared at module-scope Fixed: tint:434 Change-Id: Ia29d0f7227e838fc5f9dd4ca521b5fd6b9a88637 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/36761 Auto-Submit: David Neto Commit-Queue: dan sinclair Reviewed-by: dan sinclair --- src/reader/spirv/function.cc | 50 ++++-- src/reader/spirv/function.h | 30 +++- src/reader/spirv/parser_impl.cc | 21 ++- src/reader/spirv/parser_impl.h | 12 +- .../spirv/parser_impl_module_var_test.cc | 154 ++++++++++++++++++ 5 files changed, 240 insertions(+), 27 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index b72b55a952..9c78615fd4 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -976,6 +976,9 @@ bool FunctionEmitter::EmitBody() { return false; } + if (!RegisterIgnoredBuiltInVariables()) { + return false; + } if (!RegisterLocallyDefinedValues()) { return false; } @@ -2880,6 +2883,9 @@ bool FunctionEmitter::EmitConstDefOrWriteToHoistedVar( } bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) { + if (failed()) { + return false; + } const auto result_id = inst.result_id(); const auto type_id = inst.type_id(); @@ -2994,14 +3000,9 @@ bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) { // a new named constant definition. auto value_id = inst.GetSingleWordInOperand(0); const auto skip = GetSkipReason(value_id); - switch (skip) { - case SkipReason::kDontSkip: - break; - case SkipReason::kOpaqueObject: - case SkipReason::kPointSizeBuiltinPointer: - case SkipReason::kPointSizeBuiltinValue: - GetDefInfo(inst.result_id())->skip = skip; - return true; + if (skip != SkipReason::kDontSkip) { + GetDefInfo(inst.result_id())->skip = skip; + return true; } auto expr = MakeExpression(value_id); expr.type = RemapStorageClass(expr.type, result_id); @@ -3235,6 +3236,14 @@ TypedExpression FunctionEmitter::MakeAccessChain( return {}; } + const auto base_id = inst.GetSingleWordInOperand(0); + const auto base_skip = GetSkipReason(base_id); + if (base_skip != SkipReason::kDontSkip) { + // This can occur for AccessChain with no indices. + GetDefInfo(inst.result_id())->skip = base_skip; + return {}; + } + // A SPIR-V access chain is a single instruction with multiple indices // walking down into composites. The Tint AST represents this as // ever-deeper nested indexing expressions. Start off with an expression @@ -3242,7 +3251,6 @@ TypedExpression FunctionEmitter::MakeAccessChain( TypedExpression current_expr(MakeOperand(inst, 0)); const auto constants = constant_mgr_->GetOperandConstants(&inst); - const auto base_id = inst.GetSingleWordInOperand(0); auto ptr_ty_id = def_use_mgr_->GetDef(base_id)->type_id(); uint32_t first_index = 1; const auto num_in_operands = inst.NumInOperands(); @@ -3592,9 +3600,29 @@ TypedExpression FunctionEmitter::MakeVectorShuffle( create(source, result_type, values)}; } +bool FunctionEmitter::RegisterIgnoredBuiltInVariables() { + size_t index = def_info_.size(); + for (auto& ignored_var : parser_impl_.ignored_builtins()) { + const auto id = ignored_var.first; + const auto builtin = ignored_var.second; + def_info_[id] = + std::make_unique(*(def_use_mgr_->GetDef(id)), 0, index); + ++index; + auto& def = def_info_[id]; + switch (builtin) { + case SpvBuiltInPointSize: + def->skip = SkipReason::kPointSizeBuiltinPointer; + break; + default: + return Fail() << "unrecognized ignored builtin: " << int(builtin); + } + } + return true; +} + bool FunctionEmitter::RegisterLocallyDefinedValues() { // Create a DefInfo for each value definition in this function. - size_t index = 0; + size_t index = def_info_.size(); for (auto block_id : block_order_) { const auto* block_info = GetBlockInfo(block_id); const auto block_pos = block_info->pos; @@ -3604,7 +3632,7 @@ bool FunctionEmitter::RegisterLocallyDefinedValues() { continue; } def_info_[result_id] = std::make_unique(inst, block_pos, index); - index++; + ++index; auto& info = def_info_[result_id]; // Determine storage class for pointer values. Do this in order because diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h index c27cde5d35..9002ea8109 100644 --- a/src/reader/spirv/function.h +++ b/src/reader/spirv/function.h @@ -223,10 +223,13 @@ enum class SkipReason { kPointSizeBuiltinValue, }; -/// Bookkeeping info for a SPIR-V ID defined in the function. -/// This will be valid for result IDs for: -/// - instructions that are not OpLabel, and not OpFunctionParameter -/// - are defined in a basic block visited in the block-order for the function. +/// Bookkeeping info for a SPIR-V ID defined in the function, or some +/// module-scope variables. This will be valid for result IDs that are: +/// - defined in the function and: +/// - instructions that are not OpLabel, and not OpFunctionParameter +/// - are defined in a basic block visited in the block-order for the +/// function. +/// - certain module-scope builtin variables. struct DefInfo { /// Constructor. /// @param def_inst the SPIR-V instruction defining the ID @@ -240,8 +243,11 @@ struct DefInfo { /// The SPIR-V instruction that defines the ID. const spvtools::opt::Instruction& inst; - /// The position of the block that defines this ID, in the function block - /// order. See method `FunctionEmitter::ComputeBlockOrderAndPositions` + /// The position of the first block in which this ID is visible, in function + /// block order. For IDs defined outside of the function, it is 0. + /// For IDs defined in the function, it is the position of the block + /// containing the definition of the ID. + /// See method `FunctionEmitter::ComputeBlockOrderAndPositions` const uint32_t block_pos = 0; /// An index for uniquely and deterministically ordering all DefInfo records @@ -290,8 +296,8 @@ struct DefInfo { /// This is kNone for non-pointers. ast::StorageClass storage_class = ast::StorageClass::kNone; - /// The reason, if any, that this value should not be generated. - /// Normally all values are generated. This field can be updated while + /// The reason, if any, that this value should be ignored. + /// Normally no values are ignored. This field can be updated while /// generating code because sometimes we only discover necessary facts /// in the middle of generating code. SkipReason skip = SkipReason::kDontSkip; @@ -456,8 +462,14 @@ class FunctionEmitter { /// @returns false if bad nesting has been detected. bool FindIfSelectionInternalHeaders(); + /// Creates a DefInfo record for each module-scope builtin variable + /// that should be ignored. + /// Populates the `def_info_` mapping for such IDs. + /// @returns false on failure + bool RegisterIgnoredBuiltInVariables(); + /// Creates a DefInfo record for each locally defined SPIR-V ID. - /// Populates the `def_info_` mapping with basic results. + /// Populates the `def_info_` mapping with basic results for such IDs. /// @returns false on failure bool RegisterLocallyDefinedValues(); diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc index 3cb8646520..ee91183cf5 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/src/reader/spirv/parser_impl.cc @@ -1092,8 +1092,10 @@ bool ParserImpl::EmitScalarSpecConstants() { auto* ast_var = MakeVariable(inst.result_id(), ast::StorageClass::kNone, ast_type, true, ast_expr, std::move(spec_id_decos)); - ast_module_.AddGlobalVariable(ast_var); - scalar_spec_constants_.insert(inst.result_id()); + if (ast_var) { + ast_module_.AddGlobalVariable(ast_var); + scalar_spec_constants_.insert(inst.result_id()); + } } } return success_; @@ -1209,7 +1211,9 @@ bool ParserImpl::EmitModuleScopeVariables() { MakeVariable(var.result_id(), ast_storage_class, ast_store_type, false, ast_constructor, ast::VariableDecorationList{}); // TODO(dneto): initializers (a.k.a. constructor expression) - ast_module_.AddGlobalVariable(ast_var); + if (ast_var) { + ast_module_.AddGlobalVariable(ast_var); + } } // Emit gl_Position instead of gl_PerVertex @@ -1261,8 +1265,15 @@ ast::Variable* ParserImpl::MakeVariable( << ": has no operand"; return nullptr; } - auto ast_builtin = - enum_converter_.ToBuiltin(static_cast(deco[1])); + const auto spv_builtin = static_cast(deco[1]); + switch (spv_builtin) { + case SpvBuiltInPointSize: + ignored_builtins_[id] = spv_builtin; + return nullptr; + default: + break; + } + auto ast_builtin = enum_converter_.ToBuiltin(spv_builtin); if (ast_builtin == ast::Builtin::kNone) { return nullptr; } diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h index b34177c776..579d681ed2 100644 --- a/src/reader/spirv/parser_impl.h +++ b/src/reader/spirv/parser_impl.h @@ -287,14 +287,15 @@ class ParserImpl : Reader { bool EmitFunction(const spvtools::opt::Function& f); /// Creates an AST Variable node for a SPIR-V ID, including any attached - /// decorations. + /// decorations, unless it's an ignorable builtin variable. /// @param id the SPIR-V result ID /// @param sc the storage class, which cannot be ast::StorageClass::kNone /// @param type the type /// @param is_const if true, the variable is const /// @param constructor the variable constructor /// @param decorations the variable decorations - /// @returns a new Variable node, or null in the error case + /// @returns a new Variable node, or null in the ignorable variable case and + /// in the error case ast::Variable* MakeVariable(uint32_t id, ast::StorageClass sc, ast::type::Type* type, @@ -473,6 +474,9 @@ class ParserImpl : Reader { /// @returns the instruction, or nullptr on error const spvtools::opt::Instruction* GetInstructionForTest(uint32_t id) const; + using BuiltInsMap = std::unordered_map; + const BuiltInsMap& ignored_builtins() const { return ignored_builtins_; } + private: /// Converts a specific SPIR-V type to a Tint type. Integer case ast::type::Type* ConvertType(const spvtools::opt::analysis::Integer* int_ty); @@ -626,6 +630,10 @@ class ParserImpl : Reader { // The inferred pointer type for the given handle variable. std::unordered_map handle_type_; + + /// Maps the SPIR-V ID of a module-scope builtin variable that should be + /// ignored, to its builtin kind. + BuiltInsMap ignored_builtins_; }; } // namespace spirv diff --git a/src/reader/spirv/parser_impl_module_var_test.cc b/src/reader/spirv/parser_impl_module_var_test.cc index 1c568eb026..b39f219053 100644 --- a/src/reader/spirv/parser_impl_module_var_test.cc +++ b/src/reader/spirv/parser_impl_module_var_test.cc @@ -629,6 +629,160 @@ TEST_F(SpvModuleScopeVarParserTest, )") << module_str; } +std::string LoosePointSizePreamble() { + return R"( + OpCapability Shader + OpCapability Linkage ; so we don't have to declare an entry point + OpMemoryModel Logical Simple + + OpDecorate %1 BuiltIn PointSize + %void = OpTypeVoid + %voidfn = OpTypeFunction %void + %float = OpTypeFloat 32 + %uint = OpTypeInt 32 0 + %uint_0 = OpConstant %uint 0 + %uint_1 = OpConstant %uint 1 + %11 = OpTypePointer Output %float + %1 = OpVariable %11 Output +)"; +} + +TEST_F(SpvModuleScopeVarParserTest, BuiltinPointSize_Loose_Write1_IsErased) { + const std::string assembly = LoosePointSizePreamble() + R"( + %ptr = OpTypePointer Output %float + %one = OpConstant %float 1.0 + + %500 = OpFunction %void None %voidfn + %entry = OpLabel + OpStore %1 %one + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + EXPECT_TRUE(p->BuildAndParseInternalModule()); + EXPECT_TRUE(p->error().empty()); + const auto module_str = + Demangler().Demangle(p->get_module(), p->get_module().to_str()); + EXPECT_EQ(module_str, R"(Module{ + Function x_500 -> __void + () + { + Return{} + } +} +)") << module_str; +} + +TEST_F(SpvModuleScopeVarParserTest, BuiltinPointSize_Loose_WriteNon1_IsError) { + const std::string assembly = LoosePointSizePreamble() + R"( + %ptr = OpTypePointer Output %float + %999 = OpConstant %float 2.0 + + %500 = OpFunction %void None %voidfn + %entry = OpLabel + OpStore %1 %999 + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + EXPECT_FALSE(p->BuildAndParseInternalModule()); + EXPECT_THAT(p->error(), + HasSubstr("cannot store a value other than constant 1.0 to " + "PointSize builtin: OpStore %1 %999")); +} + +TEST_F(SpvModuleScopeVarParserTest, BuiltinPointSize_Loose_ReadReplaced) { + const std::string assembly = LoosePointSizePreamble() + R"( + %ptr = OpTypePointer Private %float + %900 = OpVariable %ptr Private + + %500 = OpFunction %void None %voidfn + %entry = OpLabel + %99 = OpLoad %float %1 + OpStore %900 %99 + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + EXPECT_TRUE(p->BuildAndParseInternalModule()); + EXPECT_TRUE(p->error().empty()); + const auto module_str = + Demangler().Demangle(p->get_module(), p->get_module().to_str()); + EXPECT_EQ(module_str, R"(Module{ + Variable{ + x_900 + private + __f32 + } + Function x_500 -> __void + () + { + Assignment{ + Identifier[not set]{x_900} + ScalarConstructor[not set]{1.000000} + } + Return{} + } +} +)") << module_str; +} + +TEST_F(SpvModuleScopeVarParserTest, + BuiltinPointSize_Loose_WriteViaCopyObjectPriorAccess_Erased) { + const std::string assembly = LoosePointSizePreamble() + R"( + %one = OpConstant %float 1.0 + + %500 = OpFunction %void None %voidfn + %entry = OpLabel + %20 = OpCopyObject %11 %1 + %100 = OpAccessChain %11 %20 + OpStore %100 %one + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + EXPECT_TRUE(p->BuildAndParseInternalModule()) << p->error(); + EXPECT_TRUE(p->error().empty()); + const auto module_str = + Demangler().Demangle(p->get_module(), p->get_module().to_str()); + EXPECT_EQ(module_str, R"(Module{ + Function x_500 -> __void + () + { + Return{} + } +} +)") << module_str; +} + +TEST_F(SpvModuleScopeVarParserTest, + BuiltinPointSize_Loose_WriteViaCopyObjectPostAccessChainErased) { + const std::string assembly = LoosePointSizePreamble() + R"( + %one = OpConstant %float 1.0 + + %500 = OpFunction %void None %voidfn + %entry = OpLabel + %100 = OpAccessChain %11 %1 + %101 = OpCopyObject %11 %100 + OpStore %101 %one + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + EXPECT_TRUE(p->BuildAndParseInternalModule()) << p->error(); + EXPECT_TRUE(p->error().empty()) << p->error(); + const auto module_str = + Demangler().Demangle(p->get_module(), p->get_module().to_str()); + EXPECT_EQ(module_str, R"(Module{ + Function x_500 -> __void + () + { + Return{} + } +} +)") << module_str; +} + TEST_F(SpvModuleScopeVarParserTest, BuiltinClipDistance_NotSupported) { const std::string assembly = PerVertexPreamble() + R"( %ptr_float = OpTypePointer Output %float