Clean up interface of node block stack after recent changes. (#3229)

- `Peek()` no longer returns a useful value, because we often don't
allocate a `NodeBlockId` until we finish building the block. Remove it
and update both its callers.
- Rename `PeekForAdd()` to `PeekOrAdd()` since it's no longer used to
get a block ID to add elements into.
- `PushForAdd()` was unused. Remove it.
- Add `NodeBlockStack::AddNode` to combine the operations of adding a
node and inserting it into a block.

toolchain/check/context.cpp

@@ -61,10 +61,8 @@ auto Context::VerifyOnFinish() -> void {
 }
 
 auto Context::AddNode(SemIR::Node node) -> SemIR::NodeId {
-  auto node_id = semantics_ir_->AddNodeInNoBlock(node);
-  node_block_stack_.AddNodeId(node_id);
-  CARBON_VLOG() << "AddNode " << node_block_stack_.Peek() << ": " << node
-                << "\n";
+  auto node_id = node_block_stack_.AddNode(node);
+  CARBON_VLOG() << "AddNode: " << node << "\n";
   return node_id;
 }
 
@@ -234,29 +232,23 @@ auto Context::AddCurrentCodeBlockToFunction() -> void {
                          .GetAsFunctionDeclaration();
   semantics_ir()
       .GetFunction(function_id)
-      .body_block_ids.push_back(node_block_stack().PeekForAdd());
+      .body_block_ids.push_back(node_block_stack().PeekOrAdd());
 }
 
 auto Context::is_current_position_reachable() -> bool {
-  switch (auto block_id = node_block_stack().Peek(); block_id.index) {
-    case SemIR::NodeBlockId::Unreachable.index: {
-      return false;
-    }
-    case SemIR::NodeBlockId::Invalid.index: {
-      return true;
-    }
-    default: {
-      // Our current position is at the end of a real block. That position is
-      // reachable unless the previous instruction is a terminator instruction.
-      const auto& block_contents = semantics_ir().GetNodeBlock(block_id);
-      if (block_contents.empty()) {
-        return true;
-      }
-      const auto& last_node = semantics_ir().GetNode(block_contents.back());
-      return last_node.kind().terminator_kind() !=
-             SemIR::TerminatorKind::Terminator;
-    }
+  if (!node_block_stack().is_current_block_reachable()) {
+    return false;
+  }
+
+  // Our current position is at the end of a reachable block. That position is
+  // reachable unless the previous instruction is a terminator instruction.
+  auto block_contents = node_block_stack().PeekCurrentBlockContents();
+  if (block_contents.empty()) {
+    return true;
   }
+  const auto& last_node = semantics_ir().GetNode(block_contents.back());
+  return last_node.kind().terminator_kind() !=
+         SemIR::TerminatorKind::Terminator;
 }
 
 auto Context::Initialize(Parse::Node parse_node, SemIR::NodeId target_id,

toolchain/check/handle_operator.cpp

@@ -36,7 +36,7 @@ auto HandleInfixOperator(Context& context, Parse::Node parse_node) -> bool {
 
       // When the second operand is evaluated, the result of `and` and `or` is
       // its value.
-      auto resume_block_id = context.node_block_stack().PeekForAdd(/*depth=*/1);
+      auto resume_block_id = context.node_block_stack().PeekOrAdd(/*depth=*/1);
       context.AddNode(SemIR::Node::BranchWithArg::Make(
           parse_node, resume_block_id, rhs_id));
       context.node_block_stack().Pop();

toolchain/check/handle_struct.cpp

@@ -51,11 +51,9 @@ auto HandleStructFieldValue(Context& context, Parse::Node parse_node) -> bool {
   value_node_id = context.ConvertToValueExpression(value_node_id);
 
   // Store the name for the type.
-  context.args_type_info_stack().AddNodeId(
-      context.semantics_ir().AddNodeInNoBlock(
-          SemIR::Node::StructTypeField::Make(
-              parse_node, name_id,
-              context.semantics_ir().GetNode(value_node_id).type_id())));
+  context.args_type_info_stack().AddNode(SemIR::Node::StructTypeField::Make(
+      parse_node, name_id,
+      context.semantics_ir().GetNode(value_node_id).type_id()));
 
   // Push the value back on the stack as an argument.
   context.node_stack().Push(parse_node, value_node_id);

toolchain/check/node_block_stack.cpp

@@ -22,13 +22,12 @@ auto NodeBlockStack::Push(SemIR::NodeBlockId id) -> void {
   ++size_;
 }
 
-auto NodeBlockStack::PeekForAdd(int depth) -> SemIR::NodeBlockId {
+auto NodeBlockStack::PeekOrAdd(int depth) -> SemIR::NodeBlockId {
   CARBON_CHECK(size() > depth) << "no such block";
   int index = size() - depth - 1;
   auto& slot = stack_[index];
   if (!slot.id.is_valid()) {
     slot.id = semantics_ir_->AddNodeBlockId();
-    CARBON_VLOG() << name_ << " Add " << index << ": " << slot.id << "\n";
   }
   return slot.id;
 }

toolchain/check/node_block_stack.h

@@ -11,7 +11,10 @@
 
 namespace Carbon::Check {
 
-// Wraps the stack of node blocks for Context.
+// A stack of node blocks that are currently being constructed in a Context. The
+// contents of the node blocks are stored here until the node block is popped
+// from the stack, at which point they are transferred into the SemIR::File for
+// long-term storage.
 //
 // All pushes and pops will be vlogged.
 class NodeBlockStack {
@@ -23,35 +26,30 @@ class NodeBlockStack {
   // Pushes an existing node block.
   auto Push(SemIR::NodeBlockId id) -> void;
 
-  // Pushes a new node block. It will be invalid unless PeekForAdd is called in
+  // Pushes a new node block. It will be invalid unless PeekOrAdd is called in
   // order to support lazy allocation.
   auto Push() -> void { Push(SemIR::NodeBlockId::Invalid); }
 
   // Pushes a new unreachable code block.
   auto PushUnreachable() -> void { Push(SemIR::NodeBlockId::Unreachable); }
 
-  // Allocates and pushes a new node block.
-  auto PushForAdd() -> SemIR::NodeBlockId {
-    Push();
-    return PeekForAdd();
-  }
-
-  // Peeks at the top node block. This does not trigger lazy allocation, so the
-  // returned node block may be invalid.
-  auto Peek() -> SemIR::NodeBlockId {
-    CARBON_CHECK(!empty()) << "no current block";
-    return stack_[size() - 1].id;
-  }
-
-  // Returns the top node block, allocating one if it's still invalid. If
+  // Returns the ID of the top node block, allocating one if necessary. If
   // `depth` is specified, returns the node at `depth` levels from the top of
-  // the stack, where the top block is at depth 0.
-  auto PeekForAdd(int depth = 0) -> SemIR::NodeBlockId;
+  // the stack instead of the top block, where the top block is at depth 0.
+  auto PeekOrAdd(int depth = 0) -> SemIR::NodeBlockId;
 
   // Pops the top node block. This will always return a valid node block;
   // SemIR::NodeBlockId::Empty is returned if one wasn't allocated.
   auto Pop() -> SemIR::NodeBlockId;
 
+  // Adds the given node to the block at the top of the stack and returns its
+  // ID.
+  auto AddNode(SemIR::Node node) -> SemIR::NodeId {
+    auto node_id = semantics_ir_->AddNodeInNoBlock(node);
+    AddNodeId(node_id);
+    return node_id;
+  }
+
   // Adds the given node ID to the block at the top of the stack.
   auto AddNodeId(SemIR::NodeId node_id) -> void {
     CARBON_CHECK(!empty()) << "no current block";
@@ -60,7 +58,14 @@ class NodeBlockStack {
 
   // Returns whether the current block is statically reachable.
   auto is_current_block_reachable() -> bool {
-    return Peek() != SemIR::NodeBlockId::Unreachable;
+    return size_ != 0 &&
+           stack_[size_ - 1].id != SemIR::NodeBlockId::Unreachable;
+  }
+
+  // Returns a view of the contents of the top node block on the stack.
+  auto PeekCurrentBlockContents() -> llvm::ArrayRef<SemIR::NodeId> {
+    CARBON_CHECK(!empty()) << "no current block";
+    return stack_[size_ - 1].content;
   }
 
   // Prints the stack for a stack dump.

toolchain/check/testdata/basics/verbose.carbon

@@ -8,7 +8,10 @@
 // NOAUTOUPDATE
 // SET-CHECK-SUBSET
 // CHECK:STDERR: Node Push 0: FunctionIntroducer -> <none>
-// CHECK:STDERR: AddNode block{{[0-9]+}}: {kind: Return}
+// CHECK:STDERR: AddNode: {kind: FunctionDeclaration, arg0: function{{[0-9]+}}}
+// CHECK:STDERR: node_block_stack_ Push 1
+// CHECK:STDERR: AddNode: {kind: Return}
+// CHECK:STDERR: node_block_stack_ Pop 1: block{{[0-9]+}}
 
 fn Foo() {
   return;

toolchain/sem_ir/file.h

@@ -151,7 +151,8 @@ class File : public Printable<File> {
 
   // Adds a node to the node list, returning an ID to reference the node. Note
   // that this doesn't add the node to any node block. Check::Context::AddNode
-  // should usually be used instead, to add the node to the current code block.
+  // or NodeBlockStack::AddNode should usually be used instead, to add the node
+  // to the current block.
   auto AddNodeInNoBlock(Node node) -> NodeId {
     NodeId node_id(nodes_.size());
     nodes_.push_back(node);