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 <richard@metafoo.co.uk>

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);

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();
 

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<BranchNode>(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<SemIR::Branch>(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<SemIR::BranchWithArg>(
           context, node_id, {.target_id = new_block_id, .arg_id = arg_id});

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);
     }

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<SemIR::Branch>(
-                {.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.

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`

toolchain/sem_ir/block_value_store.h

@@ -103,25 +103,14 @@ class BlockValueStore : public Yaml::Printable<BlockValueStore<IdT>> {
   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));
-  }
-
-  // Sets the contents of an empty block to the given content.
-  auto SetContent(IdT block_id, llvm::ArrayRef<ElementType> content) -> void {
-    CARBON_CHECK(Get(block_id).empty(),
-                 "inst block content set more than once");
-    values_.Get(block_id) = AllocateCopy(content);
+  // Allocates a copy of the given data using our slab allocator.
+  auto AllocateCopy(llvm::ArrayRef<ElementType> data)
+      -> llvm::MutableArrayRef<ElementType> {
+    auto result = AllocateUninitialized(data.size());
+    std::uninitialized_copy(data.begin(), data.end(), result.begin());
+    return result;
   }
 
- private:
-  class KeyContext;
-
   // Allocates an uninitialized array using our slab allocator.
   auto AllocateUninitialized(size_t size)
       -> llvm::MutableArrayRef<ElementType> {
@@ -133,13 +122,11 @@ class BlockValueStore : public Yaml::Printable<BlockValueStore<IdT>> {
     return llvm::MutableArrayRef<ElementType>(storage, size);
   }
 
-  // Allocates a copy of the given data using our slab allocator.
-  auto AllocateCopy(llvm::ArrayRef<ElementType> data)
-      -> llvm::MutableArrayRef<ElementType> {
-    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<IdT>& { return values_; }
+
+ private:
+  class KeyContext;
 
   llvm::BumpPtrAllocator* allocator_;
   ValueStore<IdT> values_;

toolchain/sem_ir/inst.h

@@ -452,22 +452,35 @@ class InstBlockStore : public BlockValueStore<InstBlockId> {
  public:
   using BaseType = BlockValueStore<InstBlockId>;
 
-  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<InstId> content) -> void {
-    CARBON_CHECK(block_id != InstBlockId::Unreachable);
-    BlockValueStore<InstBlockId>::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<ElementType>());
+  }
+
+  // Sets the contents of a placeholder block to the given content.
+  auto ReplacePlaceholder(InstBlockId block_id, llvm::ArrayRef<InstId> 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