From 0205645e7d5d0cb941caf5eb38c00b0bd976c792 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Thu, 4 Jan 2024 18:56:03 -0800 Subject: [PATCH] Refactor BindName to support tracking the enclosing scope. (#3566) This is a step towards adding enclosing scopes for imports. It creates an indirection for all bind names. We discussed specializing for bindings that are in function scope (i.e., not a useful enclosing scope for imports or diagnostics). However, the thought is to go ahead with this singular approach for now, and only change structure if it's a performance issues so that we have incrementally fewer instructions to handle. --- toolchain/check/check.cpp | 2 +- toolchain/check/context.cpp | 4 +++- toolchain/check/context.h | 3 +++ toolchain/check/handle_binding_pattern.cpp | 12 +++++++++--- toolchain/check/handle_function.cpp | 5 +++-- toolchain/check/handle_let.cpp | 3 ++- toolchain/check/handle_variable.cpp | 8 ++++---- .../check/testdata/basics/builtin_insts.carbon | 1 + .../basics/multifile_raw_and_textual_ir.carbon | 2 ++ .../testdata/basics/multifile_raw_ir.carbon | 2 ++ .../testdata/basics/raw_and_textual_ir.carbon | 4 +++- toolchain/check/testdata/basics/raw_ir.carbon | 4 +++- toolchain/sem_ir/file.cpp | 1 + toolchain/sem_ir/file.h | 17 +++++++++++++++++ toolchain/sem_ir/formatter.cpp | 8 +++++++- toolchain/sem_ir/ids.h | 17 +++++++++++++++++ toolchain/sem_ir/typed_insts.h | 4 +++- toolchain/sem_ir/yaml_test.cpp | 1 + 18 files changed, 82 insertions(+), 16 deletions(-) diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index cc0ee1e8703b..96cc1702d54b 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -84,7 +84,7 @@ static auto GetImportNameId(Parse::NodeId parse_node, Context& context, switch (import_inst.kind()) { case SemIR::InstKind::BindName: { auto bind_name = import_inst.As(); - return bind_name.name_id; + return import_sem_ir.bind_names().Get(bind_name.bind_name_id).name_id; } case SemIR::InstKind::FunctionDecl: { diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 1ff8a187919f..8d19738d276e 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -144,9 +144,11 @@ auto Context::AddPackageImports(Parse::NodeId import_node, // Add a name for formatted output. This isn't used in name lookup in order // to reduce indirection, but it's separate from the Import because it // otherwise fits in an Inst. + auto bind_name_id = bind_names().Add( + {.name_id = name_id, .enclosing_scope_id = SemIR::NameScopeId::Package}); AddInst(SemIR::BindName{.parse_node = import_node, .type_id = type_id, - .name_id = name_id, + .bind_name_id = bind_name_id, .value_id = inst_id}); } diff --git a/toolchain/check/context.h b/toolchain/check/context.h index 3f947c617ccc..273502391d3e 100644 --- a/toolchain/check/context.h +++ b/toolchain/check/context.h @@ -351,6 +351,9 @@ class Context { auto string_literal_values() -> StringStoreWrapper& { return sem_ir().string_literal_values(); } + auto bind_names() -> ValueStore& { + return sem_ir().bind_names(); + } auto functions() -> ValueStore& { return sem_ir().functions(); } diff --git a/toolchain/check/handle_binding_pattern.cpp b/toolchain/check/handle_binding_pattern.cpp index e400daa3c835..888b9611dfe0 100644 --- a/toolchain/check/handle_binding_pattern.cpp +++ b/toolchain/check/handle_binding_pattern.cpp @@ -13,7 +13,9 @@ auto HandleAddress(Context& context, Parse::AddressId parse_node) -> bool { auto self_param_id = context.node_stack().Pop(); auto self_param = context.insts().TryGetAs(self_param_id); - if (self_param && self_param->name_id == SemIR::NameId::SelfValue) { + if (self_param && + context.bind_names().Get(self_param->bind_name_id).name_id == + SemIR::NameId::SelfValue) { // TODO: The type of an `addr_pattern` should probably be the non-pointer // type, because that's the type that the pattern matches. context.AddInstAndPush( @@ -49,10 +51,14 @@ auto HandleBindingPattern(Context& context, Parse::BindingPatternId parse_node) // Create the appropriate kind of binding for this pattern. // // TODO: Update this to create a generic or template binding as needed. - auto make_bind_name = [name_node = name_node, name_id = name_id]( + auto make_bind_name = [&, name_node = name_node, name_id = name_id]( SemIR::TypeId type_id, SemIR::InstId value_id) -> SemIR::Inst { - return SemIR::BindName{name_node, type_id, name_id, value_id}; + // TODO: Set the correct enclosing_scope_id. + auto bind_name_id = context.bind_names().Add( + {.name_id = name_id, + .enclosing_scope_id = SemIR::NameScopeId::Invalid}); + return SemIR::BindName{name_node, type_id, bind_name_id, value_id}; }; // A `self` binding can only appear in an implicit parameter list. diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_function.cpp index b202e1ed1595..6eeb279dc29f 100644 --- a/toolchain/check/handle_function.cpp +++ b/toolchain/check/handle_function.cpp @@ -262,8 +262,9 @@ auto HandleFunctionDefinitionStart(Context& context, }); if (auto fn_param = param.TryAs()) { - context.AddNameToLookup(fn_param->parse_node, fn_param->name_id, - param_id); + context.AddNameToLookup( + fn_param->parse_node, + context.bind_names().Get(fn_param->bind_name_id).name_id, param_id); } else { CARBON_FATAL() << "Unexpected kind of parameter in function definition " << param; diff --git a/toolchain/check/handle_let.cpp b/toolchain/check/handle_let.cpp index d06422225217..a304225f4b51 100644 --- a/toolchain/check/handle_let.cpp +++ b/toolchain/check/handle_let.cpp @@ -52,7 +52,8 @@ auto HandleLetDecl(Context& context, Parse::LetDeclId parse_node) -> bool { context.inst_block_stack().AddInstId(pattern_id); // Add the name of the binding to the current scope. - context.AddNameToLookup(pattern.parse_node(), bind_name.name_id, pattern_id); + auto name_id = context.bind_names().Get(bind_name.bind_name_id).name_id; + context.AddNameToLookup(pattern.parse_node(), name_id, pattern_id); return true; } diff --git a/toolchain/check/handle_variable.cpp b/toolchain/check/handle_variable.cpp index 777d09fc1069..0783d0bed2ca 100644 --- a/toolchain/check/handle_variable.cpp +++ b/toolchain/check/handle_variable.cpp @@ -57,10 +57,10 @@ auto HandleVariableDecl(Context& context, Parse::VariableDeclId parse_node) if (auto bind_name = context.insts().Get(value_id).TryAs()) { // Form a corresponding name in the current context, and bind the name to // the variable. - context.decl_name_stack().AddNameToLookup( - context.decl_name_stack().MakeUnqualifiedName(bind_name->parse_node, - bind_name->name_id), - value_id); + auto name_context = context.decl_name_stack().MakeUnqualifiedName( + bind_name->parse_node, + context.bind_names().Get(bind_name->bind_name_id).name_id); + context.decl_name_stack().AddNameToLookup(name_context, value_id); value_id = bind_name->value_id; } diff --git a/toolchain/check/testdata/basics/builtin_insts.carbon b/toolchain/check/testdata/basics/builtin_insts.carbon index 4134d90a62a0..f784f9d119b0 100644 --- a/toolchain/check/testdata/basics/builtin_insts.carbon +++ b/toolchain/check/testdata/basics/builtin_insts.carbon @@ -10,6 +10,7 @@ // CHECK:STDOUT: filename: builtin_insts.carbon // CHECK:STDOUT: sem_ir: // CHECK:STDOUT: cross_ref_irs_size: 1 +// CHECK:STDOUT: bind_names: {} // CHECK:STDOUT: functions: {} // CHECK:STDOUT: classes: {} // CHECK:STDOUT: types: diff --git a/toolchain/check/testdata/basics/multifile_raw_and_textual_ir.carbon b/toolchain/check/testdata/basics/multifile_raw_and_textual_ir.carbon index f1ee5926e849..385f0376f2f5 100644 --- a/toolchain/check/testdata/basics/multifile_raw_and_textual_ir.carbon +++ b/toolchain/check/testdata/basics/multifile_raw_and_textual_ir.carbon @@ -22,6 +22,7 @@ fn B() {} // CHECK:STDOUT: filename: a.carbon // CHECK:STDOUT: sem_ir: // CHECK:STDOUT: cross_ref_irs_size: 1 +// CHECK:STDOUT: bind_names: {} // CHECK:STDOUT: functions: // CHECK:STDOUT: function0: {name: name0, param_refs: empty, body: [block2]} // CHECK:STDOUT: classes: {} @@ -60,6 +61,7 @@ fn B() {} // CHECK:STDOUT: filename: b.carbon // CHECK:STDOUT: sem_ir: // CHECK:STDOUT: cross_ref_irs_size: 1 +// CHECK:STDOUT: bind_names: {} // CHECK:STDOUT: functions: // CHECK:STDOUT: function0: {name: name0, param_refs: empty, body: [block2]} // CHECK:STDOUT: classes: {} diff --git a/toolchain/check/testdata/basics/multifile_raw_ir.carbon b/toolchain/check/testdata/basics/multifile_raw_ir.carbon index ff5e57cac587..57bbdbf1b88d 100644 --- a/toolchain/check/testdata/basics/multifile_raw_ir.carbon +++ b/toolchain/check/testdata/basics/multifile_raw_ir.carbon @@ -22,6 +22,7 @@ fn B() {} // CHECK:STDOUT: filename: a.carbon // CHECK:STDOUT: sem_ir: // CHECK:STDOUT: cross_ref_irs_size: 1 +// CHECK:STDOUT: bind_names: {} // CHECK:STDOUT: functions: // CHECK:STDOUT: function0: {name: name0, param_refs: empty, body: [block2]} // CHECK:STDOUT: classes: {} @@ -47,6 +48,7 @@ fn B() {} // CHECK:STDOUT: filename: b.carbon // CHECK:STDOUT: sem_ir: // CHECK:STDOUT: cross_ref_irs_size: 1 +// CHECK:STDOUT: bind_names: {} // CHECK:STDOUT: functions: // CHECK:STDOUT: function0: {name: name0, param_refs: empty, body: [block2]} // CHECK:STDOUT: classes: {} diff --git a/toolchain/check/testdata/basics/raw_and_textual_ir.carbon b/toolchain/check/testdata/basics/raw_and_textual_ir.carbon index 6da12dee0541..06929217ad66 100644 --- a/toolchain/check/testdata/basics/raw_and_textual_ir.carbon +++ b/toolchain/check/testdata/basics/raw_and_textual_ir.carbon @@ -16,6 +16,8 @@ fn Foo(n: i32) -> (i32, i32, f64) { // CHECK:STDOUT: filename: raw_and_textual_ir.carbon // CHECK:STDOUT: sem_ir: // CHECK:STDOUT: cross_ref_irs_size: 1 +// CHECK:STDOUT: bind_names: +// CHECK:STDOUT: bindName0: {name: name1, enclosing_scope: name_scope} // CHECK:STDOUT: functions: // CHECK:STDOUT: function0: {name: name0, param_refs: block2, return_type: type4, return_slot: inst+7, body: [block5]} // CHECK:STDOUT: classes: {} @@ -39,7 +41,7 @@ fn Foo(n: i32) -> (i32, i32, f64) { // CHECK:STDOUT: insts: // CHECK:STDOUT: inst+0: {kind: Namespace, arg0: name_scope0, type: type0} // CHECK:STDOUT: inst+1: {kind: Param, arg0: name1, type: type1} -// CHECK:STDOUT: inst+2: {kind: BindName, arg0: name1, arg1: inst+1, type: type1} +// CHECK:STDOUT: inst+2: {kind: BindName, arg0: bindName0, arg1: inst+1, type: type1} // CHECK:STDOUT: inst+3: {kind: TupleType, arg0: typeBlock0, type: typeTypeType} // CHECK:STDOUT: inst+4: {kind: TupleLiteral, arg0: block3, type: type2} // CHECK:STDOUT: inst+5: {kind: TupleType, arg0: typeBlock1, type: typeTypeType} diff --git a/toolchain/check/testdata/basics/raw_ir.carbon b/toolchain/check/testdata/basics/raw_ir.carbon index 821584776cfd..4999d983d004 100644 --- a/toolchain/check/testdata/basics/raw_ir.carbon +++ b/toolchain/check/testdata/basics/raw_ir.carbon @@ -16,6 +16,8 @@ fn Foo(n: i32) -> (i32, i32, f64) { // CHECK:STDOUT: filename: raw_ir.carbon // CHECK:STDOUT: sem_ir: // CHECK:STDOUT: cross_ref_irs_size: 1 +// CHECK:STDOUT: bind_names: +// CHECK:STDOUT: bindName0: {name: name1, enclosing_scope: name_scope} // CHECK:STDOUT: functions: // CHECK:STDOUT: function0: {name: name0, param_refs: block2, return_type: type4, return_slot: inst+7, body: [block5]} // CHECK:STDOUT: classes: {} @@ -39,7 +41,7 @@ fn Foo(n: i32) -> (i32, i32, f64) { // CHECK:STDOUT: insts: // CHECK:STDOUT: inst+0: {kind: Namespace, arg0: name_scope0, type: type0} // CHECK:STDOUT: inst+1: {kind: Param, arg0: name1, type: type1} -// CHECK:STDOUT: inst+2: {kind: BindName, arg0: name1, arg1: inst+1, type: type1} +// CHECK:STDOUT: inst+2: {kind: BindName, arg0: bindName0, arg1: inst+1, type: type1} // CHECK:STDOUT: inst+3: {kind: TupleType, arg0: typeBlock0, type: typeTypeType} // CHECK:STDOUT: inst+4: {kind: TupleLiteral, arg0: block3, type: type2} // CHECK:STDOUT: inst+5: {kind: TupleType, arg0: typeBlock1, type: typeTypeType} diff --git a/toolchain/sem_ir/file.cpp b/toolchain/sem_ir/file.cpp index b7ce2027b213..531ff6957f91 100644 --- a/toolchain/sem_ir/file.cpp +++ b/toolchain/sem_ir/file.cpp @@ -150,6 +150,7 @@ auto File::OutputYaml(bool include_builtins) const -> Yaml::OutputMapping { map.Add("sem_ir", Yaml::OutputMapping([&](Yaml::OutputMapping::Map map) { map.Add("cross_ref_irs_size", Yaml::OutputScalar(cross_ref_irs_.size())); + map.Add("bind_names", bind_names_.OutputYaml()); map.Add("functions", functions_.OutputYaml()); map.Add("classes", classes_.OutputYaml()); map.Add("types", types_.OutputYaml()); diff --git a/toolchain/sem_ir/file.h b/toolchain/sem_ir/file.h index 2929f5d33b5c..ec1221e89f6d 100644 --- a/toolchain/sem_ir/file.h +++ b/toolchain/sem_ir/file.h @@ -17,6 +17,16 @@ namespace Carbon::SemIR { +struct BindNameInfo : public Printable { + auto Print(llvm::raw_ostream& out) const -> void { + out << "{name: " << name_id << ", enclosing_scope: " << enclosing_scope_id + << "}"; + } + + NameId name_id; + NameScopeId enclosing_scope_id; +}; + // A function. struct Function : public Printable { auto Print(llvm::raw_ostream& out) const -> void { @@ -239,6 +249,10 @@ class File : public Printable { return value_stores_->string_literal_values(); } + auto bind_names() -> ValueStore& { return bind_names_; } + auto bind_names() const -> const ValueStore& { + return bind_names_; + } auto functions() -> ValueStore& { return functions_; } auto functions() const -> const ValueStore& { return functions_; } auto classes() -> ValueStore& { return classes_; } @@ -300,6 +314,9 @@ class File : public Printable { // TODO: If SemIR starts linking back to tokens, reuse its filename. std::string filename_; + // Storage for bind names. + ValueStore bind_names_; + // Storage for callable objects. ValueStore functions_; diff --git a/toolchain/sem_ir/formatter.cpp b/toolchain/sem_ir/formatter.cpp index d2cb97265681..9b5d2808c655 100644 --- a/toolchain/sem_ir/formatter.cpp +++ b/toolchain/sem_ir/formatter.cpp @@ -463,7 +463,9 @@ class InstNamer { break; } case BindName::Kind: { - add_inst_name_id(inst.As().name_id); + add_inst_name_id(sem_ir_.bind_names() + .Get(inst.As().bind_name_id) + .name_id); continue; } case FunctionDecl::Kind: { @@ -973,6 +975,10 @@ class Formatter { auto FormatArg(BuiltinKind kind) -> void { out_ << kind.label(); } + auto FormatArg(BindNameId id) -> void { + FormatName(sem_ir_.bind_names().Get(id).name_id); + } + auto FormatArg(FunctionId id) -> void { FormatFunctionName(id); } auto FormatArg(ClassId id) -> void { FormatClassName(id); } diff --git a/toolchain/sem_ir/ids.h b/toolchain/sem_ir/ids.h index b7e8efcb419a..59e12232d303 100644 --- a/toolchain/sem_ir/ids.h +++ b/toolchain/sem_ir/ids.h @@ -16,6 +16,7 @@ namespace Carbon::SemIR { // Forward declare indexed types, for integration with ValueStore. class File; class Inst; +struct BindNameInfo; struct Class; struct Function; struct Interface; @@ -67,6 +68,22 @@ constexpr InstId InstId::Invalid = InstId(InstId::InvalidIndex); // The package namespace will be the instruction after builtins. constexpr InstId InstId::PackageNamespace = InstId(BuiltinKind::ValidCount); +// The ID of a bind name. +struct BindNameId : public IdBase, public Printable { + using ValueType = BindNameInfo; + + // An explicitly invalid function ID. + static const BindNameId Invalid; + + using IdBase::IdBase; + auto Print(llvm::raw_ostream& out) const -> void { + out << "bindName"; + IdBase::Print(out); + } +}; + +constexpr BindNameId BindNameId::Invalid = BindNameId(BindNameId::InvalidIndex); + // The ID of a function. struct FunctionId : public IdBase, public Printable { using ValueType = Function; diff --git a/toolchain/sem_ir/typed_insts.h b/toolchain/sem_ir/typed_insts.h index dda8983061be..2b1788eb4a57 100644 --- a/toolchain/sem_ir/typed_insts.h +++ b/toolchain/sem_ir/typed_insts.h @@ -131,7 +131,9 @@ struct BindName { // TODO: Make this more specific. Parse::NodeId parse_node; TypeId type_id; - NameId name_id; + BindNameId bind_name_id; + // The value is inline in the inst so that value access doesn't require an + // indirection. InstId value_id; }; diff --git a/toolchain/sem_ir/yaml_test.cpp b/toolchain/sem_ir/yaml_test.cpp index c1b82b52c33c..18497b553c48 100644 --- a/toolchain/sem_ir/yaml_test.cpp +++ b/toolchain/sem_ir/yaml_test.cpp @@ -50,6 +50,7 @@ TEST(SemIRTest, YAML) { auto file = Yaml::Mapping(ElementsAre( Pair("cross_ref_irs_size", "1"), + Pair("bind_names", Yaml::Mapping(SizeIs(1))), Pair("functions", Yaml::Mapping(SizeIs(1))), Pair("classes", Yaml::Mapping(SizeIs(0))), Pair("types", Yaml::Mapping(Each(type_builtin))),