From 85d5b40429343a029204a2d1ddf1850c3f71b8ba Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Thu, 14 Sep 2023 09:05:05 -0700 Subject: [PATCH] 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 | 38 +++++++--------- toolchain/check/handle_operator.cpp | 2 +- toolchain/check/handle_struct.cpp | 8 ++-- toolchain/check/node_block_stack.cpp | 3 +- toolchain/check/node_block_stack.h | 43 +++++++++++-------- .../check/testdata/basics/verbose.carbon | 5 ++- toolchain/sem_ir/file.h | 3 +- 7 files changed, 50 insertions(+), 52 deletions(-) diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 8af78abc7fa6..ca240881fbc7 100644 --- a/toolchain/check/context.cpp +++ b/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, diff --git a/toolchain/check/handle_operator.cpp b/toolchain/check/handle_operator.cpp index 917a9b986896..879b6022fa74 100644 --- a/toolchain/check/handle_operator.cpp +++ b/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(); diff --git a/toolchain/check/handle_struct.cpp b/toolchain/check/handle_struct.cpp index 321eb813a907..4021e05c1f20 100644 --- a/toolchain/check/handle_struct.cpp +++ b/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); diff --git a/toolchain/check/node_block_stack.cpp b/toolchain/check/node_block_stack.cpp index dea167886631..0b3edfaf2e46 100644 --- a/toolchain/check/node_block_stack.cpp +++ b/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; } diff --git a/toolchain/check/node_block_stack.h b/toolchain/check/node_block_stack.h index 034185241c83..3a43f221022a 100644 --- a/toolchain/check/node_block_stack.h +++ b/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 { + CARBON_CHECK(!empty()) << "no current block"; + return stack_[size_ - 1].content; } // Prints the stack for a stack dump. diff --git a/toolchain/check/testdata/basics/verbose.carbon b/toolchain/check/testdata/basics/verbose.carbon index 216e8677551a..0133c4bb71d1 100644 --- a/toolchain/check/testdata/basics/verbose.carbon +++ b/toolchain/check/testdata/basics/verbose.carbon @@ -8,7 +8,10 @@ // NOAUTOUPDATE // SET-CHECK-SUBSET // CHECK:STDERR: Node Push 0: FunctionIntroducer -> -// 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; diff --git a/toolchain/sem_ir/file.h b/toolchain/sem_ir/file.h index 018d99894bef..78a80283f317 100644 --- a/toolchain/sem_ir/file.h +++ b/toolchain/sem_ir/file.h @@ -151,7 +151,8 @@ class File : public Printable { // 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);