From 7d9140feb9a7273801400c0e7166f8d994db8ce1 Mon Sep 17 00:00:00 2001 From: David Neto Date: Tue, 20 Sep 2022 15:37:19 +0000 Subject: [PATCH] spirv-reader: Refactor tracking of locally-defined values Based on suggestions from bclayton to use std::optional Change-Id: I372472dbd5e239713eee5c2ec6ae6ea05fc384fa Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/102660 Reviewed-by: Ben Clayton Commit-Queue: David Neto Kokoro: Kokoro --- src/tint/reader/spirv/function.cc | 103 +++++++++++++----------- src/tint/reader/spirv/function.h | 126 +++++++++++++++++------------- 2 files changed, 129 insertions(+), 100 deletions(-) diff --git a/src/tint/reader/spirv/function.cc b/src/tint/reader/spirv/function.cc index 59a98e183e..00b49d90d1 100644 --- a/src/tint/reader/spirv/function.cc +++ b/src/tint/reader/spirv/function.cc @@ -759,17 +759,22 @@ 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), - locally_defined(the_locally_defined), - block_pos(the_block_pos), - index(the_index) {} +DefInfo::DefInfo(size_t the_index, + const spvtools::opt::Instruction& def_inst, + uint32_t the_block_pos) + : index(the_index), inst(def_inst), local(DefInfo::Local(the_block_pos)) {} + +DefInfo::DefInfo(size_t the_index, const spvtools::opt::Instruction& def_inst) + : index(the_index), inst(def_inst) {} DefInfo::~DefInfo() = default; +DefInfo::Local::Local(uint32_t the_block_pos) : block_pos(the_block_pos) {} + +DefInfo::Local::Local(const Local& other) = default; + +DefInfo::Local::~Local() = default; + ast::Node* StatementBuilder::Clone(CloneContext*) const { return nullptr; } @@ -3380,7 +3385,7 @@ bool FunctionEmitter::EmitStatementsInBasicBlock(const BlockInfo& block_info, utils::Vector worklist; worklist.Reserve(block_info.phi_assignments.Length()); for (const auto assignment : block_info.phi_assignments) { - if (GetDefInfo(assignment.phi_id)->num_uses > 0) { + if (GetDefInfo(assignment.phi_id)->local->num_uses > 0) { worklist.Push(assignment); } } @@ -3462,7 +3467,7 @@ bool FunctionEmitter::WriteIfHoistedVar(const spvtools::opt::Instruction& inst, TypedExpression expr) { const auto result_id = inst.result_id(); const auto* def_info = GetDefInfo(result_id); - if (def_info && def_info->requires_hoisted_def) { + if (def_info && def_info->requires_hoisted_var_def) { auto name = namer_.Name(result_id); // Emit an assignment of the expression to the hoisted variable. AddStatement(create( @@ -3512,8 +3517,11 @@ bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) { } if (combinatorial_expr.expr != nullptr) { - if (def_info->requires_hoisted_def || def_info->requires_named_const_def || - def_info->num_uses != 1) { + // If the expression is combinatorial, then it's not a direct access + // of a builtin variable. + TINT_ASSERT(Reader, def_info->local.has_value()); + if (def_info->requires_hoisted_var_def || def_info->requires_named_let_def || + def_info->local->num_uses != 1) { // Generate a const definition or an assignment to a hoisted definition // now and later use the const or variable name at the uses of this // value. @@ -4739,7 +4747,7 @@ 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, false, 0, index); + def_info_[id] = std::make_unique(index, *var); ++index; auto& def = def_info_[id]; // Builtins are always defined outside the function. @@ -4787,7 +4795,7 @@ bool FunctionEmitter::RegisterLocallyDefinedValues() { if ((result_id == 0) || inst.opcode() == SpvOpLabel) { continue; } - def_info_[result_id] = std::make_unique(inst, true, block_pos, index); + def_info_[result_id] = std::make_unique(index, inst, block_pos); ++index; auto& info = def_info_[result_id]; @@ -4876,7 +4884,7 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { const auto id = inst.GetSingleWordInOperand(static_cast(in_operand_index)); auto* const operand_def = GetDefInfo(id); if (operand_def) { - operand_def->requires_named_const_def = true; + operand_def->requires_named_let_def = true; } }; for (auto& id_def_info_pair : def_info_) { @@ -4913,18 +4921,21 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { // Ignores values defined outside this function. auto record_value_use = [this](uint32_t id, const BlockInfo* block_info) { if (auto* def_info = GetDefInfo(id)) { - // Update usage count. - def_info->num_uses++; - // Update usage span. - def_info->first_use_pos = std::min(def_info->first_use_pos, block_info->pos); - def_info->last_use_pos = std::max(def_info->last_use_pos, block_info->pos); + if (def_info->local.has_value()) { + auto& local_def = def_info->local.value(); + // Update usage count. + local_def.num_uses++; + // Update usage span. + local_def.first_use_pos = std::min(local_def.first_use_pos, block_info->pos); + local_def.last_use_pos = std::max(local_def.last_use_pos, block_info->pos); - // Determine whether this ID is defined in a different construct - // from this use. - const auto defining_block = block_order_[def_info->block_pos]; - const auto* def_in_construct = GetBlockInfo(defining_block)->construct; - if (def_in_construct != block_info->construct) { - def_info->used_in_another_construct = true; + // Determine whether this ID is defined in a different construct + // from this use. + const auto defining_block = block_order_[local_def.block_pos]; + const auto* def_in_construct = GetBlockInfo(defining_block)->construct; + if (def_in_construct != block_info->construct) { + local_def.used_in_another_construct = true; + } } } }; @@ -4941,8 +4952,8 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { // in the parent block B. const auto phi_id = inst.result_id(); - auto* phi_def_info = GetDefInfo(phi_id); - phi_def_info->is_phi = true; + auto& phi_local_def = GetDefInfo(phi_id)->local.value(); + phi_local_def.is_phi = true; // Track all the places where we need to mention the variable, // so we can place its declaration. First, record the location of @@ -4962,9 +4973,9 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { // Track where P needs to be in scope. It's not an ordinary use, so don't // count it as one. const auto pred_pos = pred_block_info->pos; - phi_def_info->first_use_pos = - std::min(phi_def_info->first_use_pos, pred_pos); - phi_def_info->last_use_pos = std::max(phi_def_info->last_use_pos, pred_pos); + phi_local_def.first_use_pos = + std::min(phi_local_def.first_use_pos, pred_pos); + phi_local_def.last_use_pos = std::max(phi_local_def.last_use_pos, pred_pos); // Record the assignment that needs to occur at the end // of the predecessor block. @@ -4974,7 +4985,7 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { // Schedule the declaration of the state variable. const auto* enclosing_construct = - GetEnclosingScope(phi_def_info->first_use_pos, phi_def_info->last_use_pos); + GetEnclosingScope(phi_local_def.first_use_pos, phi_local_def.last_use_pos); GetBlockInfo(enclosing_construct->begin_id)->phis_needing_state_vars.Push(phi_id); } else { inst.ForEachInId([block_info, &record_value_use](const uint32_t* id_ptr) { @@ -4998,22 +5009,24 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { for (auto& id_def_info_pair : def_info_) { const auto def_id = id_def_info_pair.first; auto* def_info = id_def_info_pair.second.get(); - if (def_info->num_uses == 0) { - // There is no need to adjust the location of the declaration. - continue; - } - if (!def_info->locally_defined) { + if (!def_info->local.has_value()) { // Never hoist a variable declared at module scope. // This occurs for builtin variables, which are mapped to module-scope // private variables. continue; } + auto& local_def = def_info->local.value(); - const auto* def_in_construct = GetBlockInfo(block_order_[def_info->block_pos])->construct; + if (local_def.num_uses == 0) { + // There is no need to adjust the location of the declaration. + continue; + } + + const auto* def_in_construct = GetBlockInfo(block_order_[local_def.block_pos])->construct; // A definition in the first block of an kIfSelection or kSwitchSelection // occurs before the branch, and so that definition should count as // having been defined at the scope of the parent construct. - if (def_info->block_pos == def_in_construct->begin_pos) { + if (local_def.block_pos == def_in_construct->begin_pos) { if ((def_in_construct->kind == Construct::kIfSelection) || (def_in_construct->kind == Construct::kSwitchSelection)) { def_in_construct = def_in_construct->parent; @@ -5022,12 +5035,12 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { // We care about the earliest between the place of definition, and the first // use of the value. - const auto first_pos = std::min(def_info->block_pos, def_info->first_use_pos); - const auto last_use_pos = def_info->last_use_pos; + const auto first_pos = std::min(local_def.block_pos, local_def.first_use_pos); + const auto last_use_pos = local_def.last_use_pos; bool should_hoist_to_let = false; bool should_hoist_to_var = false; - if (def_info->is_phi) { + if (local_def.is_phi) { // We need to generate a variable, and assignments to that variable in // all the phi parent blocks. should_hoist_to_var = true; @@ -5041,7 +5054,7 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { // simple heuristic to avoid changing the cost of an operation // by moving it into or out of a loop, for example. if ((def_info->storage_class == ast::StorageClass::kInvalid) && - def_info->used_in_another_construct) { + local_def.used_in_another_construct) { should_hoist_to_let = true; } } @@ -5050,11 +5063,11 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() { const auto* enclosing_construct = GetEnclosingScope(first_pos, last_use_pos); if (should_hoist_to_let && (enclosing_construct == def_in_construct)) { // We can use a plain 'let' declaration. - def_info->requires_named_const_def = true; + def_info->requires_named_let_def = true; } else { // We need to make a hoisted variable definition. // TODO(dneto): Handle non-storable types, particularly pointers. - def_info->requires_hoisted_def = true; + def_info->requires_hoisted_var_def = true; auto* hoist_to_block = GetBlockInfo(enclosing_construct->begin_id); hoist_to_block->hoisted_ids.Push(def_id); } diff --git a/src/tint/reader/spirv/function.h b/src/tint/reader/spirv/function.h index ff7336e4d1..53c983c2d5 100644 --- a/src/tint/reader/spirv/function.h +++ b/src/tint/reader/spirv/function.h @@ -159,7 +159,7 @@ struct BlockInfo { /// The result IDs that this block is responsible for declaring as a /// hoisted variable. - /// @see DefInfo#requires_hoisted_def + /// @see DefInfo#requires_hoisted_var_def utils::Vector hoisted_ids; /// A PhiAssignment represents the assignment of a value to the state @@ -241,57 +241,74 @@ enum class SkipReason { /// function. /// - certain module-scope builtin variables. struct DefInfo { - /// Constructor. + /// Constructor for a locally defined value. + /// @param index an ordering index for uniqueness. /// @param def_inst the SPIR-V instruction defining the ID - /// @param locally_defined true if the defining instruction is in the function - /// @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, - bool locally_defined, - uint32_t block_pos, - size_t index); + /// @param block_pos the position of the first basic block dominated by the + /// definition + DefInfo(size_t index, const spvtools::opt::Instruction& def_inst, uint32_t block_pos); + /// Constructor for a value defined at module scope. + /// @param index an ordering index for uniqueness. + /// @param def_inst the SPIR-V instruction defining the ID + DefInfo(size_t index, const spvtools::opt::Instruction& def_inst); + /// 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; - - /// For IDs defined in the function, this is the position of the block - /// containing the definition of the ID, in function block order. - /// For IDs defined outside of the function, it is 0. - /// See method `FunctionEmitter::ComputeBlockOrderAndPositions` - const uint32_t block_pos = 0; - /// An index for uniquely and deterministically ordering all DefInfo records /// in a function. const size_t index = 0; - /// The number of uses of this ID. - uint32_t num_uses = 0; + /// The SPIR-V instruction that defines the ID. + const spvtools::opt::Instruction& inst; - /// The block position of the first use of this ID, or MAX_UINT if it is not - /// used at all. The "first" ordering is determined by the function block - /// order. The first use of an ID might be in an OpPhi that precedes the - /// definition of the ID. - /// The ID defined by an OpPhi is counted as being "used" in each of its - /// parent blocks. - uint32_t first_use_pos = std::numeric_limits::max(); - /// The block position of the last use of this ID, or 0 if it is not used - /// at all. The "last" ordering is determined by the function block order. - /// The ID defined by an OpPhi is counted as being "used" in each of its - /// parent blocks. - uint32_t last_use_pos = 0; + /// Information about a definition created inside a function. + struct Local { + /// Constructor. + /// @param block_pos the position of the basic block defining the value. + explicit Local(uint32_t block_pos); + /// Copy constructor. + /// @param other the original object to copy from. + Local(const Local& other); + /// Destructor. + ~Local(); - /// Is this value used in a construct other than the one in which it was - /// defined? - bool used_in_another_construct = false; + /// The position of the basic block defininig the value, in function + /// block order. + /// See method `FunctionEmitter::ComputeBlockOrderAndPositions` for block + /// ordering. + const uint32_t block_pos = 0; - /// True if this ID requires a WGSL 'const' definition, due to context. It + /// The number of uses of this ID. + uint32_t num_uses = 0; + + /// The block position of the first use of this ID, or MAX_UINT if it is not + /// used at all. The "first" ordering is determined by the function block + /// order. The first use of an ID might be in an OpPhi that precedes the + /// definition of the ID. + /// The ID defined by an OpPhi is counted as being "used" in each of its + /// parent blocks. + uint32_t first_use_pos = std::numeric_limits::max(); + /// The block position of the last use of this ID, or 0 if it is not used + /// at all. The "last" ordering is determined by the function block order. + /// The ID defined by an OpPhi is counted as being "used" in each of its + /// parent blocks. + uint32_t last_use_pos = 0; + + /// Is this value used in a construct other than the one in which it was + /// defined? + bool used_in_another_construct = false; + /// Is this ID an OpPhi? + bool is_phi = false; + }; + + /// Information about a definition inside the function. Populated if and only + /// if the definition actually is inside the function. + std::optional local; + + /// True if this ID requires a WGSL 'let' definition, due to context. It /// might get one anyway (so this is *not* an if-and-only-if condition). - bool requires_named_const_def = false; + bool requires_named_let_def = false; /// True if this ID must map to a WGSL variable declaration before the /// corresponding position of the ID definition in SPIR-V. This compensates @@ -306,10 +323,7 @@ struct DefInfo { /// variable. /// TODO(dneto): This works for constants of storable type, but not, for /// example, pointers. crbug.com/tint/98 - bool requires_hoisted_def = false; - - /// Is this ID an OpPhi? - bool is_phi = false; + bool requires_hoisted_var_def = false; /// The storage class to use for this value, if it is of pointer type. /// This is required to carry a storage class override from a storage @@ -336,14 +350,16 @@ 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() - << " locally_defined: " << (di.locally_defined ? "true" : "false") - << " block_pos: " << di.block_pos << " num_uses: " << di.num_uses - << " first_use_pos: " << di.first_use_pos << " 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") - << " is_phi: " << (di.is_phi ? "true" : "false") << ""; + << " inst.result_id: " << di.inst.result_id(); + if (di.local.has_value()) { + const auto& dil = di.local.value(); + o << " block_pos: " << dil.block_pos << " num_uses: " << dil.num_uses + << " first_use_pos: " << dil.first_use_pos << " last_use_pos: " << dil.last_use_pos + << " used_in_another_construct: " << (dil.used_in_another_construct ? "true" : "false") + << " is_phi: " << (dil.is_phi ? "true" : "false") << ""; + } + o << " requires_named_let_def: " << (di.requires_named_let_def ? "true" : "false") + << " requires_hoisted_var_def: " << (di.requires_hoisted_var_def ? "true" : "false"); if (di.storage_class != ast::StorageClass::kNone) { o << " sc:" << int(di.storage_class); } @@ -616,14 +632,14 @@ class FunctionEmitter { /// - When a SPIR-V instruction might use the dynamically computed value /// only once, but the WGSL code might reference it multiple times. /// For example, this occurs for the vector operands of OpVectorShuffle. - /// In this case the definition's DefInfo#requires_named_const_def property + /// In this case the definition's DefInfo#requires_named_let_def property /// is set to true. /// - When a definition and at least one of its uses are not in the /// same structured construct. - /// In this case the definition's DefInfo#requires_named_const_def property + /// In this case the definition's DefInfo#requires_named_let_def property /// is set to true. /// - When a definition is in a construct that does not enclose all the - /// uses. In this case the definition's DefInfo#requires_hoisted_def + /// uses. In this case the definition's DefInfo#requires_hoisted_var_def /// property is set to true. /// Updates the `def_info_` mapping. void FindValuesNeedingNamedOrHoistedDefinition();