diff --git a/src/tint/reader/spirv/function.cc b/src/tint/reader/spirv/function.cc index d88773e2ed..db1b0338ae 100644 --- a/src/tint/reader/spirv/function.cc +++ b/src/tint/reader/spirv/function.cc @@ -725,9 +725,13 @@ BlockInfo::BlockInfo(const spvtools::opt::BasicBlock& bb) : basic_block(&bb), id BlockInfo::~BlockInfo() = default; DefInfo::DefInfo(const spvtools::opt::Instruction& def_inst, + bool the_locally_defined, uint32_t the_block_pos, size_t the_index) - : inst(def_inst), block_pos(the_block_pos), index(the_index) {} + : inst(def_inst), + locally_defined(the_locally_defined), + block_pos(the_block_pos), + index(the_index) {} DefInfo::~DefInfo() = default; @@ -4479,9 +4483,10 @@ bool FunctionEmitter::RegisterSpecialBuiltInVariables() { const auto id = special_var.first; const auto builtin = special_var.second; const auto* var = def_use_mgr_->GetDef(id); - def_info_[id] = std::make_unique(*var, 0, index); + def_info_[id] = std::make_unique(*var, false, 0, index); ++index; auto& def = def_info_[id]; + // Builtins are always defined outside the function. switch (builtin) { case SpvBuiltInPointSize: def->skip = SkipReason::kPointSizeBuiltinPointer; @@ -4526,7 +4531,7 @@ bool FunctionEmitter::RegisterLocallyDefinedValues() { if ((result_id == 0) || inst.opcode() == SpvOpLabel) { continue; } - def_info_[result_id] = std::make_unique(inst, block_pos, index); + def_info_[result_id] = std::make_unique(inst, true, block_pos, index); ++index; auto& info = def_info_[result_id]; @@ -4722,6 +4727,13 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { // There is no need to adjust the location of the declaration. continue; } + if (!def_info->locally_defined) { + // Never hoist a variable declared at module scope. + // This occurs for builtin variables, which are mapped to module-scope + // private variables. + continue; + } + // The first use must be the at the SSA definition, because block order // respects dominance. const auto first_pos = def_info->block_pos; diff --git a/src/tint/reader/spirv/function.h b/src/tint/reader/spirv/function.h index 8eb33ab8cd..dbfd327175 100644 --- a/src/tint/reader/spirv/function.h +++ b/src/tint/reader/spirv/function.h @@ -241,14 +241,22 @@ enum class SkipReason { struct DefInfo { /// Constructor. /// @param def_inst the SPIR-V instruction defining the ID + /// @param locally_defined the SPIR-V instruction defining the ID /// @param block_pos the position of the basic block where the ID is defined. /// @param index an ordering index for this local definition - DefInfo(const spvtools::opt::Instruction& def_inst, uint32_t block_pos, size_t index); + DefInfo(const spvtools::opt::Instruction& def_inst, + bool locally_defined, + uint32_t block_pos, + size_t index); /// Destructor. ~DefInfo(); /// The SPIR-V instruction that defines the ID. const spvtools::opt::Instruction& inst; + + /// True if the definition of this ID is inside the function. + const bool locally_defined = true; + /// 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 @@ -320,8 +328,11 @@ struct DefInfo { /// @returns the ostream so calls can be chained inline std::ostream& operator<<(std::ostream& o, const DefInfo& di) { o << "DefInfo{" - << " inst.result_id: " << di.inst.result_id() << " block_pos: " << di.block_pos - << " num_uses: " << di.num_uses << " last_use_pos: " << di.last_use_pos + << " inst.result_id: " << di.inst.result_id() + << " locally_defined: " << (di.locally_defined ? "true" : "false") + << " block_pos: " << di.block_pos << " num_uses: " << di.num_uses + << " last_use_pos: " << di.last_use_pos + << " used_in_another_construct: " << (di.used_in_another_construct ? "true" : "false") << " requires_named_const_def: " << (di.requires_named_const_def ? "true" : "false") << " requires_hoisted_def: " << (di.requires_hoisted_def ? "true" : "false") << " phi_var: '" << di.phi_var << "'"; @@ -840,7 +851,7 @@ class FunctionEmitter { return info && info->pos != kInvalidBlockPos; } - /// Gets the local definition info for a result ID. + /// Gets the definition info for a result ID. /// @param id the SPIR-V ID of local definition. /// @returns the definition info for the given ID, if it exists, or nullptr DefInfo* GetDefInfo(uint32_t id) const { @@ -1274,7 +1285,9 @@ class FunctionEmitter { // Mapping from block ID to its bookkeeping info. std::unordered_map> block_info_; - // Mapping from a locally-defined result ID to its bookkeeping info. + // Mapping from a result ID to its bookkeeping info. This may be + // either a result ID defined in the function body, or the ID of a + // module-scope variable. std::unordered_map> def_info_; // Structured constructs, where enclosing constructs precede their children. diff --git a/src/tint/reader/spirv/parser_impl_module_var_test.cc b/src/tint/reader/spirv/parser_impl_module_var_test.cc index e4ac8cbb57..c88f0d600e 100644 --- a/src/tint/reader/spirv/parser_impl_module_var_test.cc +++ b/src/tint/reader/spirv/parser_impl_module_var_test.cc @@ -2537,6 +2537,68 @@ fn main(@builtin(vertex_index) x_1_param : u32) -> main_out { EXPECT_EQ(module_str, expected) << module_str; } +TEST_F(SpvModuleScopeVarParserTest, VertexIndex_UsedTwice_DifferentConstructs) { + // Test crbug.com/tint/1577 + // Builtin variables must not be hoisted. Before the fix, the reader + // would see two uses of the variable in different constructs and try + // to hoist it. Only function-local definitions should be hoisted. + const std::string assembly = VertexIndexPreamble("%uint") + R"( + %bool = OpTypeBool + %900 = OpConstantTrue %bool + %main = OpFunction %void None %voidfn + %entry = OpLabel + %2 = OpLoad %uint %1 ; used in outer selection + OpSelectionMerge %99 None + OpBranchConditional %900 %30 %99 + + %30 = OpLabel + %3 = OpLoad %uint %1 ; used in inner selection + OpSelectionMerge %40 None + OpBranchConditional %900 %35 %40 + + %35 = OpLabel + OpBranch %40 + + %40 = OpLabel + OpBranch %99 + + %99 = OpLabel + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModule()) << p->error() << assembly; + EXPECT_TRUE(p->error().empty()); + const auto module_str = test::ToString(p->program()); + const std::string expected = R"(var x_1 : u32; + +var x_5 : vec4; + +fn main_1() { + let x_2 : u32 = x_1; + if (true) { + let x_3 : u32 = x_1; + if (true) { + } + } + return; +} + +struct main_out { + @builtin(position) + x_5_1 : vec4, +} + +@vertex +fn main(@builtin(vertex_index) x_1_param : u32) -> main_out { + x_1 = x_1_param; + main_1(); + return main_out(x_5); +} +)"; + EXPECT_EQ(module_str, expected) << module_str; +} + TEST_F(SpvModuleScopeVarParserTest, VertexIndex_I32_Load_CopyObject) { const std::string assembly = VertexIndexPreamble("%int") + R"( %main = OpFunction %void None %voidfn