From d63fceff8eb913d475c6ecda2ddd5a1d491031cf Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 13 Sep 2023 13:47:53 -0700 Subject: [PATCH] Switch `GetNodeBlock` and `GetTypeBlock` to return an `ArrayRef` (#3220) This removes the risk of accidentally performing a vector copy when calling these functions, and is a preparation step towards the new node block allocation design. This required changing how we build call expressions. Instead of finishing the argument block and then later adding a return slot, we now delay finishing the argument block and checking for conversions to parameter types until after we've added the return slot to it. --- toolchain/check/context.cpp | 51 +++++++++++++------ toolchain/check/context.h | 25 +++++++-- toolchain/check/handle_call_expression.cpp | 35 ++++++++----- .../function/call/fail_not_callable.carbon | 27 ++++++++++ .../function/call/fail_param_count.carbon | 1 + .../function/call/fail_param_type.carbon | 1 + toolchain/lower/file_context.cpp | 8 +-- toolchain/sem_ir/file.h | 30 +++++++---- 8 files changed, 130 insertions(+), 48 deletions(-) create mode 100644 toolchain/check/testdata/function/call/fail_not_callable.carbon diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 1d67455714d0..78ada0838c90 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -69,6 +69,11 @@ auto Context::AddNodeToBlock(SemIR::NodeBlockId block, SemIR::Node node) return semantics_ir_->AddNode(block, node); } +auto Context::AddNodeIdToBlock(SemIR::NodeBlockId block, SemIR::NodeId node_id) + -> void { + semantics_ir_->AddNodeId(block, node_id); +} + auto Context::AddNodeAndPush(Parse::Node parse_node, SemIR::Node node) -> void { auto node_id = AddNode(node); node_stack_.Push(parse_node, node_id); @@ -436,7 +441,8 @@ auto Context::HandleDiscardedExpression(SemIR::NodeId expr_id) -> void { auto Context::ImplicitAsForArgs(Parse::Node call_parse_node, SemIR::NodeBlockId arg_refs_id, Parse::Node param_parse_node, - SemIR::NodeBlockId param_refs_id) -> bool { + SemIR::NodeBlockId param_refs_id, + bool has_return_slot) -> bool { // If both arguments and parameters are empty, return quickly. Otherwise, // we'll fetch both so that errors are consistent. if (arg_refs_id == SemIR::NodeBlockId::Empty && @@ -444,8 +450,16 @@ auto Context::ImplicitAsForArgs(Parse::Node call_parse_node, return true; } - auto& arg_refs = semantics_ir_->GetNodeBlock(arg_refs_id); - const auto& param_refs = semantics_ir_->GetNodeBlock(param_refs_id); + auto arg_refs = semantics_ir_->GetNodeBlock(arg_refs_id); + auto param_refs = semantics_ir_->GetNodeBlock(param_refs_id); + + if (has_return_slot) { + // There's no entry in the parameter block for the return slot, so ignore + // the corresponding entry in the argument block. + // TODO: Consider adding the return slot to the parameter list. + CARBON_CHECK(!arg_refs.empty()) << "missing return slot"; + arg_refs = arg_refs.drop_back(); + } // If sizes mismatch, fail early. if (arg_refs.size() != param_refs.size()) { @@ -581,14 +595,23 @@ auto Context::ParamOrArgComma(bool for_args) -> void { ParamOrArgSave(for_args); } -auto Context::ParamOrArgEnd(bool for_args, Parse::NodeKind start_kind) - -> SemIR::NodeBlockId { +auto Context::ParamOrArgEndNoPop(bool for_args, Parse::NodeKind start_kind) + -> void { if (parse_tree_->node_kind(node_stack_.PeekParseNode()) != start_kind) { ParamOrArgSave(for_args); } +} + +auto Context::ParamOrArgPop() -> SemIR::NodeBlockId { return params_or_args_stack_.Pop(); } +auto Context::ParamOrArgEnd(bool for_args, Parse::NodeKind start_kind) + -> SemIR::NodeBlockId { + ParamOrArgEndNoPop(for_args, start_kind); + return ParamOrArgPop(); +} + auto Context::ParamOrArgSave(bool for_args) -> void { auto [entry_parse_node, entry_node_id] = node_stack_.PopExpressionWithParseNode(); @@ -601,9 +624,7 @@ auto Context::ParamOrArgSave(bool for_args) -> void { } // Save the param or arg ID. - auto& params_or_args = - semantics_ir_->GetNodeBlock(params_or_args_stack_.PeekForAdd()); - params_or_args.push_back(entry_node_id); + AddNodeIdToBlock(params_or_args_stack_.PeekForAdd(), entry_node_id); } auto Context::CanonicalizeTypeImpl( @@ -641,9 +662,9 @@ auto Context::CanonicalizeTypeImpl( } // Compute a fingerprint for a tuple type, for use as a key in a folding set. -static auto ProfileTupleType(const llvm::SmallVector& type_ids, +static auto ProfileTupleType(llvm::ArrayRef type_ids, llvm::FoldingSetNodeID& canonical_id) -> void { - for (const auto& type_id : type_ids) { + for (auto type_id : type_ids) { canonical_id.AddInteger(type_id.index); } } @@ -739,18 +760,16 @@ auto Context::CanonicalizeStructType(Parse::Node parse_node, } auto Context::CanonicalizeTupleType(Parse::Node parse_node, - llvm::SmallVector&& type_ids) + llvm::ArrayRef type_ids) -> SemIR::TypeId { // Defer allocating a SemIR::TypeBlockId until we know this is a new type. auto profile_tuple = [&](llvm::FoldingSetNodeID& canonical_id) { ProfileTupleType(type_ids, canonical_id); }; auto make_tuple_node = [&] { - auto type_block_id = semantics_ir_->AddTypeBlock(); - auto& type_block = semantics_ir_->GetTypeBlock(type_block_id); - type_block = std::move(type_ids); - return AddNode(SemIR::Node::TupleType::Make( - parse_node, SemIR::TypeId::TypeType, type_block_id)); + return AddNode( + SemIR::Node::TupleType::Make(parse_node, SemIR::TypeId::TypeType, + semantics_ir_->AddTypeBlock(type_ids))); }; return CanonicalizeTypeImpl(SemIR::NodeKind::TupleType, profile_tuple, make_tuple_node); diff --git a/toolchain/check/context.h b/toolchain/check/context.h index 71d731226aa6..37859277426e 100644 --- a/toolchain/check/context.h +++ b/toolchain/check/context.h @@ -40,6 +40,10 @@ class Context { auto AddNodeToBlock(SemIR::NodeBlockId block, SemIR::Node node) -> SemIR::NodeId; + // Adds the ID of an existing node to the given block. + auto AddNodeIdToBlock(SemIR::NodeBlockId block, SemIR::NodeId node_id) + -> void; + // Pushes a parse tree node onto the stack, storing the SemIR::Node as the // result. auto AddNodeAndPush(Parse::Node parse_node, SemIR::Node node) -> void; @@ -148,7 +152,8 @@ class Context { auto ImplicitAsForArgs(Parse::Node call_parse_node, SemIR::NodeBlockId arg_refs_id, Parse::Node param_parse_node, - SemIR::NodeBlockId param_refs_id) -> bool; + SemIR::NodeBlockId param_refs_id, bool has_return_slot) + -> bool; // Canonicalizes a type which is tracked as a single node. // TODO: This should eventually return a type ID. @@ -167,7 +172,7 @@ class Context { // Handles canonicalization of tuple types. This may create a new tuple type // if the `type_ids` doesn't match an existing tuple type. auto CanonicalizeTupleType(Parse::Node parse_node, - llvm::SmallVector&& type_ids) + llvm::ArrayRef type_ids) -> SemIR::TypeId; // Returns a pointer type whose pointee type is `pointee_type_id`. @@ -200,9 +205,19 @@ class Context { // start_kind. auto ParamOrArgComma(bool for_args) -> void; - // Detects whether there's an entry to push. On return, the top of - // node_stack_ will be start_kind, and the caller should do type-specific - // processing. Returns refs_id. + // Detects whether there's an entry to push from the end of a parameter or + // argument list, and if so, moves it to the current parameter or argument + // list. Does not pop the list. `start_kind` is the node kind at the start + // of the parameter or argument list, and will be at the top of the parse node + // stack when this function returns. + auto ParamOrArgEndNoPop(bool for_args, Parse::NodeKind start_kind) -> void; + + // Pops the current parameter or argument list. Should only be called after + // `ParamOrArgEndNoPop`. + auto ParamOrArgPop() -> SemIR::NodeBlockId; + + // Detects whether there's an entry to push. Pops and returns the argument + // list. This is the same as `ParamOrArgEndNoPop` followed by `ParamOrArgPop`. auto ParamOrArgEnd(bool for_args, Parse::NodeKind start_kind) -> SemIR::NodeBlockId; diff --git a/toolchain/check/handle_call_expression.cpp b/toolchain/check/handle_call_expression.cpp index 81efbca6f494..db4774c2e78e 100644 --- a/toolchain/check/handle_call_expression.cpp +++ b/toolchain/check/handle_call_expression.cpp @@ -8,7 +8,9 @@ namespace Carbon::Check { auto HandleCallExpression(Context& context, Parse::Node parse_node) -> bool { - auto refs_id = context.ParamOrArgEnd( + // Process the final explicit call argument, but leave the arguments block on + // the stack until we add the return slot argument. + context.ParamOrArgEndNoPop( /*for_args=*/true, Parse::NodeKind::CallExpressionStart); // TODO: Convert to call expression. @@ -20,19 +22,13 @@ auto HandleCallExpression(Context& context, Parse::Node parse_node) -> bool { // TODO: Work on error. context.TODO(parse_node, "Not a callable name"); context.node_stack().Push(parse_node, name_id); + context.ParamOrArgPop(); return true; } auto function_id = name_node.GetAsFunctionDeclaration(); const auto& callable = context.semantics_ir().GetFunction(function_id); - if (!context.ImplicitAsForArgs(call_expr_parse_node, refs_id, - name_node.parse_node(), - callable.param_refs_id)) { - context.node_stack().Push(parse_node, SemIR::NodeId::BuiltinError); - return true; - } - // For functions with an implicit return type, the return type is the empty // tuple type. SemIR::TypeId type_id = callable.return_type_id; @@ -42,15 +38,26 @@ auto HandleCallExpression(Context& context, Parse::Node parse_node) -> bool { // If there is a return slot, add a corresponding argument. if (callable.return_slot_id.is_valid()) { - if (refs_id == SemIR::NodeBlockId::Empty) { - refs_id = context.semantics_ir().AddNodeBlock(); - } // Tentatively put storage for a temporary in the function's return slot. // This will be replaced if necessary when we perform initialization. - auto return_slot_id = context.AddNode(SemIR::Node::TemporaryStorage::Make( - call_expr_parse_node, callable.return_type_id)); - context.semantics_ir().GetNodeBlock(refs_id).push_back(return_slot_id); + context.AddNodeAndPush(parse_node, + SemIR::Node::TemporaryStorage::Make( + call_expr_parse_node, callable.return_type_id)); + // TODO: We pass for_args=false here because we don't want a StubReference. + // We should remove the StubReferences for arguments and the corresponding + // `for_args` parameter. They didn't turn out to be used for anything. + context.ParamOrArgSave(/*for_args=*/false); } + + // Convert the arguments to match the parameters. + auto refs_id = context.ParamOrArgPop(); + if (!context.ImplicitAsForArgs(call_expr_parse_node, refs_id, + name_node.parse_node(), callable.param_refs_id, + callable.return_slot_id.is_valid())) { + context.node_stack().Push(parse_node, SemIR::NodeId::BuiltinError); + return true; + } + auto call_node_id = context.AddNode(SemIR::Node::Call::Make( call_expr_parse_node, type_id, refs_id, function_id)); diff --git a/toolchain/check/testdata/function/call/fail_not_callable.carbon b/toolchain/check/testdata/function/call/fail_not_callable.carbon new file mode 100644 index 000000000000..c0511f5abf95 --- /dev/null +++ b/toolchain/check/testdata/function/call/fail_not_callable.carbon @@ -0,0 +1,27 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// AUTOUPDATE + +fn Run() { + // CHECK:STDERR: fail_not_callable.carbon:[[@LINE+6]]:24: Semantics TODO: `Not a callable name`. + // CHECK:STDERR: var x: i32 = "hello"(); + // CHECK:STDERR: ^ + // CHECK:STDERR: fail_not_callable.carbon:[[@LINE+3]]:25: Cannot implicitly convert from `String` to `i32`. + // CHECK:STDERR: var x: i32 = "hello"(); + // CHECK:STDERR: ^ + var x: i32 = "hello"(); +} + +// CHECK:STDOUT: file "fail_not_callable.carbon" { +// CHECK:STDOUT: %.loc7 = fn_decl @Run +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @Run() { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %x: ref i32 = var "x" +// CHECK:STDOUT: %.loc14: String = string_literal "hello" +// CHECK:STDOUT: assign %x, +// CHECK:STDOUT: return +// CHECK:STDOUT: } diff --git a/toolchain/check/testdata/function/call/fail_param_count.carbon b/toolchain/check/testdata/function/call/fail_param_count.carbon index b454b4f9e9ce..ca3c5ed44778 100644 --- a/toolchain/check/testdata/function/call/fail_param_count.carbon +++ b/toolchain/check/testdata/function/call/fail_param_count.carbon @@ -81,6 +81,7 @@ fn Main() { // CHECK:STDOUT: !entry: // CHECK:STDOUT: %.loc18_8.1: i32 = int_literal 1 // CHECK:STDOUT: %.loc18_8.2: i32 = stub_reference %.loc18_8.1 +// CHECK:STDOUT: %.loc18_7: type = tuple_type () // CHECK:STDOUT: %.loc25_8.1: i32 = int_literal 0 // CHECK:STDOUT: %.loc25_8.2: i32 = stub_reference %.loc25_8.1 // CHECK:STDOUT: %.loc25_11.1: i32 = int_literal 1 diff --git a/toolchain/check/testdata/function/call/fail_param_type.carbon b/toolchain/check/testdata/function/call/fail_param_type.carbon index 64525025dd1c..0f05507759d5 100644 --- a/toolchain/check/testdata/function/call/fail_param_type.carbon +++ b/toolchain/check/testdata/function/call/fail_param_type.carbon @@ -30,5 +30,6 @@ fn Main() { // CHECK:STDOUT: !entry: // CHECK:STDOUT: %.loc16_7.1: f64 = real_literal 10e-1 // CHECK:STDOUT: %.loc16_7.2: f64 = stub_reference %.loc16_7.1 +// CHECK:STDOUT: %.loc16_6: type = tuple_type () // CHECK:STDOUT: return // CHECK:STDOUT: } diff --git a/toolchain/lower/file_context.cpp b/toolchain/lower/file_context.cpp index 868409b59d8b..7f19d31928d2 100644 --- a/toolchain/lower/file_context.cpp +++ b/toolchain/lower/file_context.cpp @@ -59,7 +59,7 @@ auto FileContext::BuildFunctionDeclaration(SemIR::FunctionId function_id) -> llvm::Function* { const auto& function = semantics_ir().GetFunction(function_id); const bool has_return_slot = function.return_slot_id.is_valid(); - auto& param_refs = semantics_ir().GetNodeBlock(function.param_refs_id); + auto param_refs = semantics_ir().GetNodeBlock(function.param_refs_id); SemIR::InitializingRepresentation return_rep = function.return_type_id.is_valid() @@ -147,7 +147,7 @@ auto FileContext::BuildFunctionDefinition(SemIR::FunctionId function_id) // TODO: This duplicates the mapping between semantics nodes and LLVM // function parameters that was already computed in BuildFunctionDeclaration. // We should only do that once. - auto& param_refs = semantics_ir().GetNodeBlock(function.param_refs_id); + auto param_refs = semantics_ir().GetNodeBlock(function.param_refs_id); int param_index = 0; if (has_return_slot) { function_lowering.SetLocal(function.return_slot_id, @@ -219,7 +219,7 @@ auto FileContext::BuildType(SemIR::NodeId node_id) -> llvm::Type* { case SemIR::NodeKind::PointerType: return llvm::PointerType::get(*llvm_context_, /*AddressSpace=*/0); case SemIR::NodeKind::StructType: { - auto& refs = semantics_ir_->GetNodeBlock(node.GetAsStructType()); + auto refs = semantics_ir_->GetNodeBlock(node.GetAsStructType()); llvm::SmallVector subtypes; subtypes.reserve(refs.size()); for (auto ref_id : refs) { @@ -238,7 +238,7 @@ auto FileContext::BuildType(SemIR::NodeId node_id) -> llvm::Type* { // can be collectively replaced with LLVM's void, particularly around // function returns. LLVM doesn't allow declaring variables with a void // type, so that may require significant special casing. - auto& refs = semantics_ir_->GetTypeBlock(node.GetAsTupleType()); + auto refs = semantics_ir_->GetTypeBlock(node.GetAsTupleType()); llvm::SmallVector subtypes; subtypes.reserve(refs.size()); for (auto ref_id : refs) { diff --git a/toolchain/sem_ir/file.h b/toolchain/sem_ir/file.h index 7abcc5e4c83b..3154a64e2a61 100644 --- a/toolchain/sem_ir/file.h +++ b/toolchain/sem_ir/file.h @@ -152,10 +152,15 @@ class File : public Printable { auto AddNode(NodeBlockId block_id, Node node) -> NodeId { NodeId node_id(nodes_.size()); nodes_.push_back(node); + AddNodeId(block_id, node_id); + return node_id; + } + + // Adds the ID of an existing node to the specified block. + auto AddNodeId(NodeBlockId block_id, NodeId node_id) -> void { if (block_id != NodeBlockId::Unreachable) { node_blocks_[block_id.index].push_back(node_id); } - return node_id; } // Overwrites a given node with a new value. @@ -173,15 +178,22 @@ class File : public Printable { return id; } + // Adds a node block with the given content, returning an ID to reference it. + auto AddNodeBlock(llvm::ArrayRef content) -> NodeBlockId { + NodeBlockId id(node_blocks_.size()); + node_blocks_.push_back({}); + node_blocks_.back().assign(content.begin(), content.end()); + return id; + } + // Returns the requested node block. - auto GetNodeBlock(NodeBlockId block_id) const - -> const llvm::SmallVector& { + auto GetNodeBlock(NodeBlockId block_id) const -> llvm::ArrayRef { CARBON_CHECK(block_id != NodeBlockId::Unreachable); return node_blocks_[block_id.index]; } // Returns the requested node block. - auto GetNodeBlock(NodeBlockId block_id) -> llvm::SmallVector& { + auto GetNodeBlock(NodeBlockId block_id) -> llvm::MutableArrayRef { CARBON_CHECK(block_id != NodeBlockId::Unreachable); return node_blocks_[block_id.index]; } @@ -245,21 +257,21 @@ class File : public Printable { } } - // Adds an empty type block, returning an ID to reference it. - auto AddTypeBlock() -> TypeBlockId { + // Adds a type block with the given content, returning an ID to reference it. + auto AddTypeBlock(llvm::ArrayRef content) -> TypeBlockId { TypeBlockId id(type_blocks_.size()); type_blocks_.push_back({}); + type_blocks_.back().assign(content.begin(), content.end()); return id; } // Returns the requested type block. - auto GetTypeBlock(TypeBlockId block_id) const - -> const llvm::SmallVector& { + auto GetTypeBlock(TypeBlockId block_id) const -> llvm::ArrayRef { return type_blocks_[block_id.index]; } // Returns the requested type block. - auto GetTypeBlock(TypeBlockId block_id) -> llvm::SmallVector& { + auto GetTypeBlock(TypeBlockId block_id) -> llvm::MutableArrayRef { return type_blocks_[block_id.index]; }