From c0ee446cecb5aa47e9e3d58099c9e36e2b50e500 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 21 Mar 2025 16:09:37 -0700 Subject: [PATCH] Refactor InstBlockStore's API, AddDefaultValue -> AddPlaceholder (#5166) `AddDefaultValue` doesn't quite capture the intended semantics; it should typically be replaced with an actual value when dealing with control flows. Trying to indicate the "assign later" with `AddPlaceholder`, mirroring `AddPlaceholderInst`. Shifting the `protected` functionality on `BlockValueStore` so that it's not providing functions just for `InstBlockStore` to use. Also hoping that seeing the comments next to the function name makes them easier to understand, whereas `using` buries that a little. --------- Co-authored-by: Richard Smith --- toolchain/base/value_store.h | 7 ------ toolchain/check/check_unit.cpp | 7 +++--- toolchain/check/control_flow.cpp | 6 ++--- toolchain/check/inst_block_stack.cpp | 4 ++-- toolchain/check/subpattern.cpp | 2 +- toolchain/docs/idioms.md | 2 -- toolchain/sem_ir/block_value_store.h | 35 +++++++++------------------- toolchain/sem_ir/inst.h | 31 +++++++++++++++++------- 8 files changed, 43 insertions(+), 51 deletions(-) diff --git a/toolchain/base/value_store.h b/toolchain/base/value_store.h index 8446748e6a3a..d61d5295e8f9 100644 --- a/toolchain/base/value_store.h +++ b/toolchain/base/value_store.h @@ -62,13 +62,6 @@ class ValueStore return id; } - // Adds a default constructed value and returns an ID to reference it. - auto AddDefaultValue() -> IdT { - IdT id(values_.size()); - values_.resize(id.index + 1); - return id; - } - // Returns a mutable value for an ID. auto Get(IdT id) -> RefType { CARBON_DCHECK(id.index >= 0, "{0}", id); diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp index 187567fb0b6b..84bde8ac9439 100644 --- a/toolchain/check/check_unit.cpp +++ b/toolchain/check/check_unit.cpp @@ -515,10 +515,11 @@ auto CheckUnit::FinishRun() -> void { context_.scope_stack().Pop(); // Finalizes the list of exports on the IR. - context_.inst_blocks().Set(SemIR::InstBlockId::Exports, context_.exports()); + context_.inst_blocks().ReplacePlaceholder(SemIR::InstBlockId::Exports, + context_.exports()); // Finalizes the ImportRef inst block. - context_.inst_blocks().Set(SemIR::InstBlockId::ImportRefs, - context_.import_ref_ids()); + context_.inst_blocks().ReplacePlaceholder(SemIR::InstBlockId::ImportRefs, + context_.import_ref_ids()); // Finalizes __global_init. context_.global_init().Finalize(); diff --git a/toolchain/check/control_flow.cpp b/toolchain/check/control_flow.cpp index fddc62505385..20118868c84f 100644 --- a/toolchain/check/control_flow.cpp +++ b/toolchain/check/control_flow.cpp @@ -16,7 +16,7 @@ static auto AddDominatedBlockAndBranchImpl(Context& context, if (!context.inst_block_stack().is_current_block_reachable()) { return SemIR::InstBlockId::Unreachable; } - auto block_id = context.inst_blocks().AddDefaultValue(); + auto block_id = context.inst_blocks().AddPlaceholder(); AddInst(context, node_id, {block_id, args...}); return block_id; } @@ -47,7 +47,7 @@ auto AddConvergenceBlockAndPush(Context& context, Parse::NodeId node_id, for ([[maybe_unused]] auto _ : llvm::seq(num_blocks)) { if (context.inst_block_stack().is_current_block_reachable()) { if (new_block_id == SemIR::InstBlockId::Unreachable) { - new_block_id = context.inst_blocks().AddDefaultValue(); + new_block_id = context.inst_blocks().AddPlaceholder(); } CARBON_CHECK(node_id.has_value()); AddInst(context, node_id, {.target_id = new_block_id}); @@ -67,7 +67,7 @@ auto AddConvergenceBlockWithArgAndPush( for (auto arg_id : block_args) { if (context.inst_block_stack().is_current_block_reachable()) { if (new_block_id == SemIR::InstBlockId::Unreachable) { - new_block_id = context.inst_blocks().AddDefaultValue(); + new_block_id = context.inst_blocks().AddPlaceholder(); } AddInst( context, node_id, {.target_id = new_block_id, .arg_id = arg_id}); diff --git a/toolchain/check/inst_block_stack.cpp b/toolchain/check/inst_block_stack.cpp index 62d43bae799e..8c3d49c7b902 100644 --- a/toolchain/check/inst_block_stack.cpp +++ b/toolchain/check/inst_block_stack.cpp @@ -29,7 +29,7 @@ auto InstBlockStack::PeekOrAdd(int depth) -> SemIR::InstBlockId { int index = id_stack_.size() - depth - 1; auto& slot = id_stack_[index]; if (!slot.has_value()) { - slot = sem_ir_->inst_blocks().AddDefaultValue(); + slot = sem_ir_->inst_blocks().AddPlaceholder(); } return slot; } @@ -42,7 +42,7 @@ auto InstBlockStack::Pop() -> SemIR::InstBlockId { // Finalize the block. if (!insts.empty() && id != SemIR::InstBlockId::Unreachable) { if (id.has_value()) { - sem_ir_->inst_blocks().Set(id, insts); + sem_ir_->inst_blocks().ReplacePlaceholder(id, insts); } else { id = sem_ir_->inst_blocks().Add(insts); } diff --git a/toolchain/check/subpattern.cpp b/toolchain/check/subpattern.cpp index dac74acf6fe5..a99507193992 100644 --- a/toolchain/check/subpattern.cpp +++ b/toolchain/check/subpattern.cpp @@ -20,7 +20,7 @@ auto EndSubpatternAsExpr(Context& context, SemIR::InstId result_id) // will be determined later. AddInst(context, SemIR::LocIdAndInst::NoLoc( - {.target_id = context.inst_blocks().AddDefaultValue()})); + {.target_id = context.inst_blocks().AddPlaceholder()})); } else { // This single-block region will be inserted as a SpliceBlock, so we don't // need control flow out of it. diff --git a/toolchain/docs/idioms.md b/toolchain/docs/idioms.md index 784e1ae7ed3a..d12a0399d225 100644 --- a/toolchain/docs/idioms.md +++ b/toolchain/docs/idioms.md @@ -145,8 +145,6 @@ The indices typically use `IdBase`. `ValueStore`s APIs follow the shape of simple array access and mutation: - `Add` which takes a value and returns the index. -- `AddDefaultValue` which adds a default-constructed value and returns the - index. - `Get` takes an index and returns a reference to the value (possibly a constant reference). - Other vector-like functionality, including `size` or `Reserve` diff --git a/toolchain/sem_ir/block_value_store.h b/toolchain/sem_ir/block_value_store.h index fbc34f31e80f..a8dc2d35495b 100644 --- a/toolchain/sem_ir/block_value_store.h +++ b/toolchain/sem_ir/block_value_store.h @@ -103,25 +103,14 @@ class BlockValueStore : public Yaml::Printable> { auto size() const -> int { return values_.size(); } protected: - // Reserves and returns a block ID. The contents of the block - // should be specified by calling Set, or similar. - auto AddDefaultValue() -> IdT { return values_.AddDefaultValue(); } - - // Adds an uninitialized block of the given size. - auto AddUninitialized(size_t size) -> IdT { - return values_.Add(AllocateUninitialized(size)); + // Allocates a copy of the given data using our slab allocator. + auto AllocateCopy(llvm::ArrayRef data) + -> llvm::MutableArrayRef { + auto result = AllocateUninitialized(data.size()); + std::uninitialized_copy(data.begin(), data.end(), result.begin()); + return result; } - // Sets the contents of an empty block to the given content. - auto SetContent(IdT block_id, llvm::ArrayRef content) -> void { - CARBON_CHECK(Get(block_id).empty(), - "inst block content set more than once"); - values_.Get(block_id) = AllocateCopy(content); - } - - private: - class KeyContext; - // Allocates an uninitialized array using our slab allocator. auto AllocateUninitialized(size_t size) -> llvm::MutableArrayRef { @@ -133,13 +122,11 @@ class BlockValueStore : public Yaml::Printable> { return llvm::MutableArrayRef(storage, size); } - // Allocates a copy of the given data using our slab allocator. - auto AllocateCopy(llvm::ArrayRef data) - -> llvm::MutableArrayRef { - auto result = AllocateUninitialized(data.size()); - std::uninitialized_copy(data.begin(), data.end(), result.begin()); - return result; - } + // Allow children to have more complex value handling. + auto values() -> ValueStore& { return values_; } + + private: + class KeyContext; llvm::BumpPtrAllocator* allocator_; ValueStore values_; diff --git a/toolchain/sem_ir/inst.h b/toolchain/sem_ir/inst.h index abd90cb7befb..07c753276976 100644 --- a/toolchain/sem_ir/inst.h +++ b/toolchain/sem_ir/inst.h @@ -452,22 +452,35 @@ class InstBlockStore : public BlockValueStore { public: using BaseType = BlockValueStore; - using BaseType::AddDefaultValue; - using BaseType::AddUninitialized; - explicit InstBlockStore(llvm::BumpPtrAllocator& allocator) : BaseType(allocator) { - auto exports_id = AddDefaultValue(); + auto exports_id = AddPlaceholder(); CARBON_CHECK(exports_id == InstBlockId::Exports); - auto import_refs_id = AddDefaultValue(); + auto import_refs_id = AddPlaceholder(); CARBON_CHECK(import_refs_id == InstBlockId::ImportRefs); - auto global_init_id = AddDefaultValue(); + auto global_init_id = AddPlaceholder(); CARBON_CHECK(global_init_id == InstBlockId::GlobalInit); } - auto Set(InstBlockId block_id, llvm::ArrayRef content) -> void { - CARBON_CHECK(block_id != InstBlockId::Unreachable); - BlockValueStore::SetContent(block_id, content); + // Adds an uninitialized block of the given size. The caller is expected to + // modify values. + auto AddUninitialized(size_t size) -> InstBlockId { + return values().Add(AllocateUninitialized(size)); + } + + // Reserves and returns a block ID. The contents of the block should be + // specified by calling ReplacePlaceholder. + auto AddPlaceholder() -> InstBlockId { + return values().Add(llvm::MutableArrayRef()); + } + + // Sets the contents of a placeholder block to the given content. + auto ReplacePlaceholder(InstBlockId block_id, llvm::ArrayRef content) + -> void { + CARBON_CHECK(block_id != SemIR::InstBlockId::Empty); + CARBON_CHECK(Get(block_id).empty(), + "inst block content set more than once"); + values().Get(block_id) = AllocateCopy(content); } // Returns the contents of the specified block, or an empty array if the block