From 891c7d8368860757362fda8e5dfafd59fafb4ed0 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Thu, 29 Aug 2024 14:10:14 -0700 Subject: [PATCH] Enforce that the parse node for an instruction has the kind specified in the instruction definition (#4264) Remove `ReusingLoc` and add enforcement that even for imported locations, the kind of the parse node for an instruction matches the kind specified in the instruction definition. Change the node kind for a few instructions to `NodeId`: - A couple of instructions had a typed node but could be created implicitly with any node as part of a builtin implicit conversion. This happened for `AddrOf`, `ArrayIndex`, and `Deref`. - A bunch of instructions had `InvalidNodeId` as their associated parse node kind but were actually always created with a location. --------- Co-authored-by: Jon Ross-Perkins --- toolchain/check/check.cpp | 7 ++- toolchain/check/context.cpp | 28 ++++++++++ toolchain/check/context.h | 45 ++++++++------- toolchain/check/convert.cpp | 85 ++++++++++++++-------------- toolchain/check/generic.cpp | 8 +-- toolchain/check/handle_alias.cpp | 2 +- toolchain/check/impl.cpp | 2 +- toolchain/check/import.cpp | 34 ++++++++---- toolchain/check/import_ref.cpp | 38 +++++++------ toolchain/check/interface.cpp | 2 +- toolchain/check/member_access.cpp | 2 +- toolchain/check/pending_block.h | 26 +++++---- toolchain/sem_ir/inst.h | 34 ++++++------ toolchain/sem_ir/typed_insts.h | 92 ++++++++++++++----------------- 14 files changed, 220 insertions(+), 185 deletions(-) diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index bf23252d180f1..61ccd6b9f08b4 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -282,9 +282,10 @@ static auto ImportOtherPackages(Context& context, UnitInfo& unit_info, auto import_ir_inst_id = context.import_ir_insts().Add( {.ir_id = SemIR::ImportIRId::ApiForImpl, .inst_id = api_imports->import_decl_id}); - import_decl_id = context.AddInstReusingLoc( - import_ir_inst_id, {.package_id = SemIR::NameId::ForIdentifier( - api_imports_entry.first)}); + import_decl_id = + context.AddInst(context.MakeImportedLocAndInst( + import_ir_inst_id, {.package_id = SemIR::NameId::ForIdentifier( + api_imports_entry.first)})); package_id = api_imports_entry.first; } has_load_error |= api_imports->has_load_error; diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 15b82d9bb57c7..87b27316664aa 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -118,6 +118,34 @@ auto Context::FinishInst(SemIR::InstId inst_id, SemIR::Inst inst) -> void { } } +// Returns whether a parse node associated with an imported instruction of kind +// `imported_kind` is usable as the location of a corresponding local +// instruction of kind `local_kind`. +static auto HasCompatibleImportedNodeKind(SemIR::InstKind imported_kind, + SemIR::InstKind local_kind) -> bool { + if (imported_kind == local_kind) { + return true; + } + if (imported_kind == SemIR::ImportDecl::Kind && + local_kind == SemIR::Namespace::Kind) { + static_assert( + std::is_convertible_v); + return true; + } + return false; +} + +auto Context::CheckCompatibleImportedNodeKind( + SemIR::ImportIRInstId imported_loc_id, SemIR::InstKind kind) -> void { + auto& import_ir_inst = import_ir_insts().Get(imported_loc_id); + const auto* import_ir = import_irs().Get(import_ir_inst.ir_id).sem_ir; + auto imported_kind = import_ir->insts().Get(import_ir_inst.inst_id).kind(); + CARBON_CHECK(HasCompatibleImportedNodeKind(imported_kind, kind)) + << "Node of kind " << kind + << " created with location of imported node of kind " << imported_kind; +} + auto Context::AddInstInNoBlock(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId { auto inst_id = sem_ir().insts().AddInNoBlock(loc_id_and_inst); diff --git a/toolchain/check/context.h b/toolchain/check/context.h index b2e2b80dbacff..a82720d33411e 100644 --- a/toolchain/check/context.h +++ b/toolchain/check/context.h @@ -24,6 +24,7 @@ #include "toolchain/sem_ir/ids.h" #include "toolchain/sem_ir/import_ir.h" #include "toolchain/sem_ir/inst.h" +#include "toolchain/sem_ir/typed_insts.h" namespace Carbon::Check { @@ -68,18 +69,23 @@ class Context { auto AddInst(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId; // Convenience for AddInst with typed nodes. - template - requires(SemIR::Internal::HasNodeId) - auto AddInst(decltype(InstT::Kind)::TypedNodeId node_id, InstT inst) - -> SemIR::InstId { - return AddInst(SemIR::LocIdAndInst(node_id, inst)); + template + auto AddInst(LocT loc, InstT inst) + -> decltype(AddInst(SemIR::LocIdAndInst(loc, inst))) { + return AddInst(SemIR::LocIdAndInst(loc, inst)); } - // Convenience for AddInst when reusing a location, which any instruction can - // do. + // Returns a LocIdAndInst for an instruction with an imported location. Checks + // that the imported location is compatible with the kind of instruction being + // created. template - auto AddInstReusingLoc(SemIR::LocId loc_id, InstT inst) -> SemIR::InstId { - return AddInst(SemIR::LocIdAndInst::ReusingLoc(loc_id, inst)); + requires SemIR::Internal::HasNodeId + auto MakeImportedLocAndInst(SemIR::ImportIRInstId imported_loc_id, InstT inst) + -> SemIR::LocIdAndInst { + if constexpr (!SemIR::Internal::HasUntypedNodeId) { + CheckCompatibleImportedNodeKind(imported_loc_id, InstT::Kind); + } + return SemIR::LocIdAndInst::UncheckedLoc(imported_loc_id, inst); } // Adds an instruction in no block, returning the produced ID. Should be used @@ -87,18 +93,10 @@ class Context { auto AddInstInNoBlock(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId; // Convenience for AddInstInNoBlock with typed nodes. - template - requires(SemIR::Internal::HasNodeId) - auto AddInstInNoBlock(decltype(InstT::Kind)::TypedNodeId node_id, InstT inst) - -> SemIR::InstId { - return AddInstInNoBlock(SemIR::LocIdAndInst(node_id, inst)); - } - - // Convenience for AddInstInNoBlock on imported instructions. - template - auto AddInstInNoBlock(SemIR::ImportIRInstId import_ir_inst_id, InstT inst) - -> SemIR::InstId { - return AddInstInNoBlock(SemIR::LocIdAndInst(import_ir_inst_id, inst)); + template + auto AddInstInNoBlock(LocT loc, InstT inst) + -> decltype(AddInstInNoBlock(SemIR::LocIdAndInst(loc, inst))) { + return AddInstInNoBlock(SemIR::LocIdAndInst(loc, inst)); } // Adds an instruction to the current block, returning the produced ID. The @@ -500,6 +498,11 @@ class Context { SemIR::TypeId type_id_; }; + // Checks that the provided imported location has a node kind that is + // compatible with that of the given instruction. + auto CheckCompatibleImportedNodeKind(SemIR::ImportIRInstId imported_loc_id, + SemIR::InstKind kind) -> void; + // Finish producing an instruction. Set its constant value, and register it in // any applicable instruction lists. auto FinishInst(SemIR::InstId inst_id, SemIR::Inst inst) -> void; diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp index f942b501b0caf..baa0d3a02801e 100644 --- a/toolchain/check/convert.cpp +++ b/toolchain/check/convert.cpp @@ -104,10 +104,10 @@ static auto FinalizeTemporary(Context& context, SemIR::InstId init_id, << "initialized multiple times? Have " << sem_ir.insts().Get(return_slot_id); auto init = sem_ir.insts().Get(init_id); - return context.AddInstReusingLoc( - sem_ir.insts().GetLocId(init_id), {.type_id = init.type_id(), - .storage_id = return_slot_id, - .init_id = init_id}); + return context.AddInst(sem_ir.insts().GetLocId(init_id), + {.type_id = init.type_id(), + .storage_id = return_slot_id, + .init_id = init_id}); } if (discarded) { @@ -122,12 +122,11 @@ static auto FinalizeTemporary(Context& context, SemIR::InstId init_id, // instructions. auto init = sem_ir.insts().Get(init_id); auto loc_id = sem_ir.insts().GetLocId(init_id); - auto temporary_id = context.AddInstReusingLoc( + auto temporary_id = context.AddInst( loc_id, {.type_id = init.type_id()}); - return context.AddInstReusingLoc( - loc_id, {.type_id = init.type_id(), - .storage_id = temporary_id, - .init_id = init_id}); + return context.AddInst(loc_id, {.type_id = init.type_id(), + .storage_id = temporary_id, + .init_id = init_id}); } // Materialize a temporary to hold the result of the given expression if it is @@ -151,14 +150,14 @@ static auto MakeElementAccessInst(Context& context, SemIR::LocId loc_id, // TODO: Add a new instruction kind for indexing an array at a constant // index so that we don't need an integer literal instruction here, and // remove this special case. - auto index_id = block.template AddInstReusingLoc( + auto index_id = block.template AddInst( loc_id, {.type_id = context.GetBuiltinType(SemIR::BuiltinInstKind::IntType), .int_id = context.ints().Add(llvm::APInt(32, i))}); - return block.template AddInstReusingLoc( + return block.template AddInst( loc_id, {elem_type_id, aggregate_id, index_id}); } else { - return block.template AddInstReusingLoc( + return block.template AddInst( loc_id, {elem_type_id, aggregate_id, SemIR::ElementIndex(i)}); } } @@ -257,7 +256,7 @@ static auto ConvertTupleToArray(Context& context, SemIR::TupleType tuple_type, // destination for the array initialization if we weren't given one. SemIR::InstId return_slot_id = target.init_id; if (!target.init_id.is_valid()) { - return_slot_id = target_block->AddInstReusingLoc( + return_slot_id = target_block->AddInst( value_loc_id, {.type_id = target.type_id}); } @@ -284,7 +283,7 @@ static auto ConvertTupleToArray(Context& context, SemIR::TupleType tuple_type, // Flush the temporary here if we didn't insert it earlier, so we can add a // reference to the return slot. target_block->InsertHere(); - return context.AddInstReusingLoc( + return context.AddInst( value_loc_id, {.type_id = target.type_id, .inits_id = sem_ir.inst_blocks().Add(inits), .dest_id = return_slot_id}); @@ -364,12 +363,12 @@ static auto ConvertTupleToTuple(Context& context, SemIR::TupleType src_type, if (is_init) { target.init_block->InsertHere(); - return context.AddInstReusingLoc( - value_loc_id, {.type_id = target.type_id, - .elements_id = new_block.id(), - .dest_id = target.init_id}); + return context.AddInst(value_loc_id, + {.type_id = target.type_id, + .elements_id = new_block.id(), + .dest_id = target.init_id}); } else { - return context.AddInstReusingLoc( + return context.AddInst( value_loc_id, {.type_id = target.type_id, .elements_id = new_block.id()}); } @@ -498,18 +497,18 @@ static auto ConvertStructToStructOrClass(Context& context, target.init_block->InsertHere(); CARBON_CHECK(is_init) << "Converting directly to a class value is not supported"; - return context.AddInstReusingLoc( - value_loc_id, {.type_id = target.type_id, - .elements_id = new_block.id(), - .dest_id = target.init_id}); + return context.AddInst(value_loc_id, + {.type_id = target.type_id, + .elements_id = new_block.id(), + .dest_id = target.init_id}); } else if (is_init) { target.init_block->InsertHere(); - return context.AddInstReusingLoc( - value_loc_id, {.type_id = target.type_id, - .elements_id = new_block.id(), - .dest_id = target.init_id}); + return context.AddInst(value_loc_id, + {.type_id = target.type_id, + .elements_id = new_block.id(), + .dest_id = target.init_id}); } else { - return context.AddInstReusingLoc( + return context.AddInst( value_loc_id, {.type_id = target.type_id, .elements_id = new_block.id()}); } @@ -556,7 +555,7 @@ static auto ConvertStructToClass(Context& context, SemIR::StructType src_type, if (need_temporary) { target.kind = ConversionTarget::Initializer; target.init_block = &target_block; - target.init_id = target_block.AddInstReusingLoc( + target.init_id = target_block.AddInst( context.insts().GetLocId(value_id), {.type_id = target.type_id}); } @@ -565,7 +564,7 @@ static auto ConvertStructToClass(Context& context, SemIR::StructType src_type, if (need_temporary) { target_block.InsertHere(); - result_id = context.AddInstReusingLoc( + result_id = context.AddInst( context.insts().GetLocId(value_id), {.type_id = target.type_id, .storage_id = target.init_id, .init_id = result_id}); @@ -623,7 +622,7 @@ static auto ConvertDerivedToBase(Context& context, SemIR::LocId loc_id, // Add a series of `.base` accesses. for (auto base_id : path) { auto base_decl = context.insts().GetAs(base_id); - value_id = context.AddInstReusingLoc( + value_id = context.AddInst( loc_id, {.type_id = base_decl.base_type_id, .base_id = value_id, .index = base_decl.index}); @@ -638,14 +637,14 @@ static auto ConvertDerivedPointerToBasePointer( const InheritancePath& path) -> SemIR::InstId { // Form `*p`. ptr_id = ConvertToValueExpr(context, ptr_id); - auto ref_id = context.AddInstReusingLoc( + auto ref_id = context.AddInst( loc_id, {.type_id = src_ptr_type.pointee_id, .pointer_id = ptr_id}); // Convert as a reference expression. ref_id = ConvertDerivedToBase(context, loc_id, ref_id, path); // Take the address. - return context.AddInstReusingLoc( + return context.AddInst( loc_id, {.type_id = dest_ptr_type_id, .lvalue_id = ref_id}); } @@ -744,7 +743,7 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id, // The initializer produces an object representation by copy, and the // value representation is a copy of the object representation, so we // already have a value of the right form. - return context.AddInstReusingLoc( + return context.AddInst( loc_id, {.type_id = value_type_id, .init_id = value_id}); } } @@ -765,7 +764,7 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id, ConversionTarget{.kind = ConversionTarget::Value, .type_id = value_type_id}); } - return context.AddInstReusingLoc( + return context.AddInst( loc_id, {.type_id = target.type_id, .source_id = value_id}); } } @@ -871,7 +870,7 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id, // TODO: Support converting tuple and struct values to facet types, // combining the above conversions and this one in a single conversion. if (sem_ir.types().Is(value_type_id)) { - return context.AddInstReusingLoc( + return context.AddInst( loc_id, {.type_id = target.type_id, .facet_id = value_id}); } } @@ -982,10 +981,10 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, // Track that we performed a type conversion, if we did so. if (orig_expr_id != expr_id) { - expr_id = context.AddInstReusingLoc( - loc_id, {.type_id = target.type_id, - .original_id = orig_expr_id, - .result_id = expr_id}); + expr_id = + context.AddInst(loc_id, {.type_id = target.type_id, + .original_id = orig_expr_id, + .result_id = expr_id}); } // For `as`, don't perform any value category conversions. In particular, an @@ -1035,7 +1034,7 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, // If we have a reference and don't want one, form a value binding. // TODO: Support types with custom value representations. - expr_id = context.AddInstReusingLoc( + expr_id = context.AddInst( context.insts().GetLocId(expr_id), {.type_id = expr.type_id(), .value_id = expr_id}); // We now have a value expression. @@ -1054,7 +1053,7 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id, if (auto init_rep = SemIR::InitRepr::ForType(sem_ir, target.type_id); init_rep.kind == SemIR::InitRepr::ByCopy) { target.init_block->InsertHere(); - expr_id = context.AddInstReusingLoc( + expr_id = context.AddInst( loc_id, {.type_id = target.type_id, .src_id = expr_id, .dest_id = target.init_id}); @@ -1163,7 +1162,7 @@ static auto ConvertSelf(Context& context, SemIR::LocId call_loc_id, return SemIR::InstId::BuiltinError; } auto loc_id = context.insts().GetLocId(self_or_addr_id); - self_or_addr_id = context.AddInstReusingLoc( + self_or_addr_id = context.AddInst( loc_id, {.type_id = context.GetPointerType(self.type_id()), .lvalue_id = self_or_addr_id}); } diff --git a/toolchain/check/generic.cpp b/toolchain/check/generic.cpp index 6ad9a5f594ffd..594b8d5582094 100644 --- a/toolchain/check/generic.cpp +++ b/toolchain/check/generic.cpp @@ -431,11 +431,9 @@ auto RequireGenericParams(Context& context, SemIR::InstBlockId block_id) // constructing a generic based on it. Note this is updating the param // refs block, not the actual params block, so will not be directly // reflected in SemIR output. - inst_id = context.AddInstInNoBlock( - SemIR::LocIdAndInst::ReusingLoc( - context.insts().GetLocId(inst_id), - {.type_id = SemIR::TypeId::Error, - .name_id = SemIR::NameId::Base})); + inst_id = context.AddInstInNoBlock( + context.insts().GetLocId(inst_id), + {.type_id = SemIR::TypeId::Error, .name_id = SemIR::NameId::Base}); } } } diff --git a/toolchain/check/handle_alias.cpp b/toolchain/check/handle_alias.cpp index 9cebc59055c25..3b6d88bf813fe 100644 --- a/toolchain/check/handle_alias.cpp +++ b/toolchain/check/handle_alias.cpp @@ -59,7 +59,7 @@ auto HandleParseNode(Context& context, Parse::AliasId /*node_id*/) -> bool { alias_type_id = SemIR::TypeId::Error; alias_value_id = SemIR::InstId::BuiltinError; } - auto alias_id = context.AddInstReusingLoc( + auto alias_id = context.AddInst( name_context.loc_id, {.type_id = alias_type_id, .entity_name_id = entity_name_id, .value_id = alias_value_id}); diff --git a/toolchain/check/impl.cpp b/toolchain/check/impl.cpp index db7854887abf6..29f78a6ddafcf 100644 --- a/toolchain/check/impl.cpp +++ b/toolchain/check/impl.cpp @@ -198,7 +198,7 @@ static auto BuildInterfaceWitness( } auto table_id = context.inst_blocks().Add(table); - return context.AddInstReusingLoc( + return context.AddInst( context.insts().GetLocId(impl.definition_id), {.type_id = context.GetBuiltinType(SemIR::BuiltinInstKind::WitnessType), .elements_id = table_id}); diff --git a/toolchain/check/import.cpp b/toolchain/check/import.cpp index 51b13c97ae3c9..b5fe10812a26d 100644 --- a/toolchain/check/import.cpp +++ b/toolchain/check/import.cpp @@ -84,10 +84,11 @@ static auto CopyNameFromImportIR(Context& context, // conflict. diagnose_duplicate_namespace is used when handling a cross-package // import, where an existing namespace is in the current package and the new // namespace is a different package. -static auto AddNamespace( - Context& context, SemIR::TypeId namespace_type_id, SemIR::NameId name_id, - SemIR::NameScopeId parent_scope_id, bool diagnose_duplicate_namespace, - std::optional> make_import_id) +static auto AddNamespace(Context& context, SemIR::TypeId namespace_type_id, + SemIR::NameId name_id, + SemIR::NameScopeId parent_scope_id, + bool diagnose_duplicate_namespace, + llvm::function_ref make_import_id) -> std::tuple { auto* parent_scope = &context.name_scopes().Get(parent_scope_id); auto insert_result = @@ -97,7 +98,7 @@ static auto AddNamespace( if (auto namespace_inst = context.insts().TryGetAs(prev_inst_id)) { if (diagnose_duplicate_namespace) { - auto import_id = (*make_import_id)(); + auto import_id = make_import_id(); CARBON_CHECK(import_id.is_valid()); context.DiagnoseDuplicateName(import_id, prev_inst_id); } @@ -105,13 +106,21 @@ static auto AddNamespace( } } - auto import_id = (*make_import_id)(); + auto import_id = make_import_id(); CARBON_CHECK(import_id.is_valid()); + auto import_loc_id = context.insts().GetLocId(import_id); + auto namespace_inst = SemIR::Namespace{ namespace_type_id, SemIR::NameScopeId::Invalid, import_id}; + auto namespace_inst_and_loc = + import_loc_id.is_import_ir_inst_id() + ? context.MakeImportedLocAndInst(import_loc_id.import_ir_inst_id(), + namespace_inst) + // TODO: Check that this actually is an `AnyNamespaceId`. + : SemIR::LocIdAndInst(Parse::AnyNamespaceId(import_loc_id.node_id()), + namespace_inst); auto namespace_id = - context.AddPlaceholderInstInNoBlock(SemIR::LocIdAndInst::ReusingLoc( - context.insts().GetLocId(import_id), namespace_inst)); + context.AddPlaceholderInstInNoBlock(namespace_inst_and_loc); context.import_ref_ids().push_back(namespace_id); namespace_inst.name_scope_id = context.name_scopes().Add(namespace_id, name_id, parent_scope_id); @@ -165,10 +174,11 @@ static auto CopySingleNameScopeFromImportIR( .bind_index = SemIR::CompileTimeBindIndex::Invalid}); auto import_ir_inst_id = context.import_ir_insts().Add( {.ir_id = ir_id, .inst_id = import_inst_id}); - auto inst_id = context.AddInstInNoBlock( - import_ir_inst_id, {.type_id = namespace_type_id, - .import_ir_inst_id = import_ir_inst_id, - .entity_name_id = entity_name_id}); + auto inst_id = context.AddInstInNoBlock( + context.MakeImportedLocAndInst( + import_ir_inst_id, {.type_id = namespace_type_id, + .import_ir_inst_id = import_ir_inst_id, + .entity_name_id = entity_name_id})); context.import_ref_ids().push_back(inst_id); return inst_id; }; diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index 95b236ee2b0bd..ece423c882e26 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -60,7 +60,7 @@ auto AddImportRef(Context& context, SemIR::ImportIRInst import_ir_inst, SemIR::ImportRefUnloaded inst = {.import_ir_inst_id = import_ir_inst_id, .entity_name_id = entity_name_id}; auto import_ref_id = context.AddPlaceholderInstInNoBlock( - SemIR::LocIdAndInst(import_ir_inst_id, inst)); + context.MakeImportedLocAndInst(import_ir_inst_id, inst)); // ImportRefs have a dedicated block because this may be called during // processing where the instruction shouldn't be inserted in the current inst @@ -741,9 +741,10 @@ class ImportRefResolver { } } if (addr_inst) { - new_param_id = context_.AddInstInNoBlock( - AddImportIRInst(ref_id), - {.type_id = type_id, .inner_id = new_param_id}); + new_param_id = context_.AddInstInNoBlock( + context_.MakeImportedLocAndInst( + AddImportIRInst(ref_id), + {.type_id = type_id, .inner_id = new_param_id})); } new_param_refs.push_back(new_param_id); } @@ -1157,11 +1158,13 @@ class ImportRefResolver { // Import the instruction in order to update contained base_type_id and // track the import location. - auto inst_id = context_.AddInstInNoBlock( - AddImportIRInst(import_inst_id), - {.type_id = context_.GetTypeIdForTypeConstant(type_const_id), - .base_type_id = context_.GetTypeIdForTypeConstant(base_type_const_id), - .index = inst.index}); + auto inst_id = context_.AddInstInNoBlock( + context_.MakeImportedLocAndInst( + AddImportIRInst(import_inst_id), + {.type_id = context_.GetTypeIdForTypeConstant(type_const_id), + .base_type_id = + context_.GetTypeIdForTypeConstant(base_type_const_id), + .index = inst.index})); return ResolveAsConstant(context_.constant_values().Get(inst_id)); } @@ -1198,7 +1201,7 @@ class ImportRefResolver { .class_id = SemIR::ClassId::Invalid, .decl_block_id = SemIR::InstBlockId::Empty}; auto class_decl_id = - context_.AddPlaceholderInstInNoBlock(SemIR::LocIdAndInst( + context_.AddPlaceholderInstInNoBlock(context_.MakeImportedLocAndInst( AddImportIRInst(import_class.latest_decl_id()), class_decl)); // Regardless of whether ClassDecl is a complete type, we first need an // incomplete type so that any references have something to point at. @@ -1371,11 +1374,12 @@ class ImportRefResolver { if (HasNewWork()) { return Retry(); } - auto inst_id = context_.AddInstInNoBlock( - AddImportIRInst(import_inst_id), - {.type_id = context_.GetTypeIdForTypeConstant(const_id), - .name_id = GetLocalNameId(inst.name_id), - .index = inst.index}); + auto inst_id = context_.AddInstInNoBlock( + context_.MakeImportedLocAndInst( + AddImportIRInst(import_inst_id), + {.type_id = context_.GetTypeIdForTypeConstant(const_id), + .name_id = GetLocalNameId(inst.name_id), + .index = inst.index})); return {.const_id = context_.constant_values().Get(inst_id)}; } @@ -1389,7 +1393,7 @@ class ImportRefResolver { .function_id = SemIR::FunctionId::Invalid, .decl_block_id = SemIR::InstBlockId::Empty}; auto function_decl_id = - context_.AddPlaceholderInstInNoBlock(SemIR::LocIdAndInst( + context_.AddPlaceholderInstInNoBlock(context_.MakeImportedLocAndInst( AddImportIRInst(import_function.first_decl_id()), function_decl)); // Start with an incomplete function. @@ -1558,7 +1562,7 @@ class ImportRefResolver { .interface_id = SemIR::InterfaceId::Invalid, .decl_block_id = SemIR::InstBlockId::Empty}; auto interface_decl_id = - context_.AddPlaceholderInstInNoBlock(SemIR::LocIdAndInst( + context_.AddPlaceholderInstInNoBlock(context_.MakeImportedLocAndInst( AddImportIRInst(import_interface.first_owning_decl_id), interface_decl)); diff --git a/toolchain/check/interface.cpp b/toolchain/check/interface.cpp index 519bb89c5b3f3..5bf4fca1cee01 100644 --- a/toolchain/check/interface.cpp +++ b/toolchain/check/interface.cpp @@ -34,7 +34,7 @@ auto BuildAssociatedEntity(Context& context, SemIR::InterfaceId interface_id, // not the declaration itself. auto type_id = context.GetAssociatedEntityType( self_type_id, context.insts().Get(decl_id).type_id()); - return context.AddInstReusingLoc( + return context.AddInst( context.insts().GetLocId(decl_id), {.type_id = type_id, .index = index, .decl_id = decl_id}); } diff --git a/toolchain/check/member_access.cpp b/toolchain/check/member_access.cpp index d65a59f01b57b..f43bcf3540a70 100644 --- a/toolchain/check/member_access.cpp +++ b/toolchain/check/member_access.cpp @@ -196,7 +196,7 @@ static auto PerformImplLookup(Context& context, Parse::NodeId node_id, auto subst_type_id = SemIR::GetTypeInSpecific( context.sem_ir(), interface_type.specific_id, assoc_type.entity_type_id); - return context.AddInstReusingLoc( + return context.AddInst( node_id, {.type_id = subst_type_id, .witness_id = witness_id, .index = assoc_entity->index}); diff --git a/toolchain/check/pending_block.h b/toolchain/check/pending_block.h index 8a2fbab5dedc2..8999cbd591a48 100644 --- a/toolchain/check/pending_block.h +++ b/toolchain/check/pending_block.h @@ -40,10 +40,9 @@ class PendingBlock { size_t size_; }; - template - auto AddInstReusingLoc(SemIR::LocId loc_id, InstT inst) -> SemIR::InstId { - auto inst_id = context_.AddInstInNoBlock( - SemIR::LocIdAndInst::ReusingLoc(loc_id, inst)); + template + auto AddInst(LocT loc_id, InstT inst) -> SemIR::InstId { + auto inst_id = context_.AddInstInNoBlock(loc_id, inst); insts_.push_back(inst_id); return inst_id; } @@ -67,10 +66,12 @@ class PendingBlock { // 1) The block is empty. Replace `target_id` with an empty splice // pointing at `value_id`. context_.ReplaceLocIdAndInstBeforeConstantUse( - target_id, SemIR::LocIdAndInst::ReusingLoc( - value.loc_id, {.type_id = value.inst.type_id(), - .block_id = SemIR::InstBlockId::Empty, - .result_id = value_id})); + target_id, + SemIR::LocIdAndInst( + value.loc_id, + SemIR::SpliceBlock{.type_id = value.inst.type_id(), + .block_id = SemIR::InstBlockId::Empty, + .result_id = value_id})); } else if (insts_.size() == 1 && insts_[0] == value_id) { // 2) The block is {value_id}. Replace `target_id` with the instruction // referred to by `value_id`. This is intended to be the common case. @@ -79,10 +80,11 @@ class PendingBlock { // 3) Anything else: splice it into the IR, replacing `target_id`. context_.ReplaceLocIdAndInstBeforeConstantUse( target_id, - SemIR::LocIdAndInst::ReusingLoc( - value.loc_id, {.type_id = value.inst.type_id(), - .block_id = context_.inst_blocks().Add(insts_), - .result_id = value_id})); + SemIR::LocIdAndInst( + value.loc_id, + SemIR::SpliceBlock{.type_id = value.inst.type_id(), + .block_id = context_.inst_blocks().Add(insts_), + .result_id = value_id})); } // Prepare to stash more pending instructions. diff --git a/toolchain/sem_ir/inst.h b/toolchain/sem_ir/inst.h index 82a9fd8a132c5..e929b86930d71 100644 --- a/toolchain/sem_ir/inst.h +++ b/toolchain/sem_ir/inst.h @@ -305,19 +305,19 @@ inline auto operator<<(llvm::raw_ostream& out, TypedInst inst) // Associates a LocId and Inst in order to provide type-checking that the // TypedNodeId corresponds to the InstT. struct LocIdAndInst { - // Constructs a LocIdAndInst with no associated location. Note, we should - // generally do our best to associate a location for diagnostics. + // Constructs a LocIdAndInst with no associated location. This should be used + // very sparingly: only when it doesn't make sense to store a location even + // when the instruction kind usually has one, such as for instructions in the + // constants block. template static auto NoLoc(InstT inst) -> LocIdAndInst { - return LocIdAndInst(LocId::Invalid, inst, /*is_untyped=*/true); + return LocIdAndInst(LocId::Invalid, inst, /*is_unchecked=*/true); } - // Constructs a LocIdAndInst that reuses the location associated with some - // other inst, typically because `inst` doesn't have an explicit - // representation in the parse tree. - template - static auto ReusingLoc(LocId loc_id, InstT inst) -> LocIdAndInst { - return LocIdAndInst(loc_id, inst, /*is_untyped=*/true); + // Unsafely form a pair of a location and an instruction. Used in the cases + // where we can't statically enforce the type matches. + static auto UncheckedLoc(LocId loc_id, Inst inst) -> LocIdAndInst { + return LocIdAndInst(loc_id, inst, /*is_unchecked=*/true); } // Construction for the common case with a typed node. @@ -326,20 +326,18 @@ struct LocIdAndInst { LocIdAndInst(decltype(InstT::Kind)::TypedNodeId node_id, InstT inst) : loc_id(node_id), inst(inst) {} - // Imports can pass an ImportIRInstId instead of another location. + // Construction for the case where the instruction can have any associated + // node. template - LocIdAndInst(ImportIRInstId import_ir_inst_id, InstT inst) - : loc_id(import_ir_inst_id), inst(inst) {} + requires(Internal::HasUntypedNodeId) + LocIdAndInst(SemIR::LocId loc_id, InstT inst) : loc_id(loc_id), inst(inst) {} LocId loc_id; Inst inst; private: - // Expose the internal constructor for GetWithLocId. - friend class InstStore; - - // Note `is_untyped` serves to disambiguate from public constructors. - explicit LocIdAndInst(LocId loc_id, Inst inst, bool /*is_untyped*/) + // Note `is_unchecked` serves to disambiguate from public constructors. + explicit LocIdAndInst(LocId loc_id, Inst inst, bool /*is_unchecked*/) : loc_id(loc_id), inst(inst) {} }; @@ -361,7 +359,7 @@ class InstStore { // Returns the requested instruction and its location ID. auto GetWithLocId(InstId inst_id) const -> LocIdAndInst { - return LocIdAndInst(GetLocId(inst_id), Get(inst_id), /*is_untyped=*/true); + return LocIdAndInst::UncheckedLoc(GetLocId(inst_id), Get(inst_id)); } // Returns whether the requested instruction is the specified type. diff --git a/toolchain/sem_ir/typed_insts.h b/toolchain/sem_ir/typed_insts.h index 99ad99399a9dd..161e81fd389d0 100644 --- a/toolchain/sem_ir/typed_insts.h +++ b/toolchain/sem_ir/typed_insts.h @@ -53,12 +53,12 @@ struct AdaptDecl { TypeId adapted_type_id; }; -// The `&` address-of operator, as in `&lvalue`. +// Takes the address of a reference expression, such as for the `&` address-of +// operator, `&lvalue`. struct AddrOf { - static constexpr auto Kind = - InstKind::AddrOf.Define( - {.ir_name = "addr_of", - .constant_kind = InstConstantKind::Conditional}); + // Parse node is usually Parse::PrefixOperatorAmpId. + static constexpr auto Kind = InstKind::AddrOf.Define( + {.ir_name = "addr_of", .constant_kind = InstConstantKind::Conditional}); TypeId type_id; InstId lvalue_id; @@ -77,8 +77,9 @@ struct AddrPattern { // An array indexing operation, such as `array[index]`. struct ArrayIndex { - static constexpr auto Kind = InstKind::ArrayIndex.Define( - {.ir_name = "array_index"}); + // Parse node is usually Parse::IndexExprId. + static constexpr auto Kind = + InstKind::ArrayIndex.Define({.ir_name = "array_index"}); TypeId type_id; InstId array_id; @@ -136,8 +137,8 @@ struct AnyAggregateValue { // expression. `inits_id` contains one initializer per array element. // `dest_id` is the destination array object for the initialization. struct ArrayInit { - static constexpr auto Kind = InstKind::ArrayInit.Define( - {.ir_name = "array_init"}); + static constexpr auto Kind = + InstKind::ArrayInit.Define({.ir_name = "array_init"}); TypeId type_id; InstBlockId inits_id; @@ -158,9 +159,8 @@ struct ArrayType { // Perform a no-op conversion to a compatible type. struct AsCompatible { - static constexpr auto Kind = - InstKind::AsCompatible.Define( - {.ir_name = "as_compatible"}); + static constexpr auto Kind = InstKind::AsCompatible.Define( + {.ir_name = "as_compatible"}); TypeId type_id; InstId source_id; @@ -194,10 +194,8 @@ struct AssociatedConstantDecl { // This represents the entity before impl lookup is performed, and identifies // the slot within a witness where the constant value will be found. struct AssociatedEntity { - static constexpr auto Kind = - InstKind::AssociatedEntity.Define( - {.ir_name = "assoc_entity", - .constant_kind = InstConstantKind::Always}); + static constexpr auto Kind = InstKind::AssociatedEntity.Define( + {.ir_name = "assoc_entity", .constant_kind = InstConstantKind::Always}); // The type of the associated entity. This is an AssociatedEntityType. TypeId type_id; @@ -294,7 +292,6 @@ struct BindSymbolicName { // A value binding. Used when an expression contains a reference and we want a // value. struct BindValue { - // TODO: Make Parse::NodeId more specific. static constexpr auto Kind = InstKind::BindValue.Define({.ir_name = "bind_value"}); @@ -304,7 +301,6 @@ struct BindValue { // Reads an argument from `BranchWithArg`. struct BlockArg { - // TODO: Make Parse::NodeId more specific. static constexpr auto Kind = InstKind::BlockArg.Define({.ir_name = "block_arg"}); @@ -444,8 +440,8 @@ struct ClassElementAccess { // Initializes a class object at dest_id with the contents of elements_id. struct ClassInit { - static constexpr auto Kind = InstKind::ClassInit.Define( - {.ir_name = "class_init"}); + static constexpr auto Kind = + InstKind::ClassInit.Define({.ir_name = "class_init"}); TypeId type_id; InstBlockId elements_id; @@ -479,8 +475,8 @@ struct ConstType { // Records that a type conversion `original as new_type` was done, producing the // result. struct Converted { - static constexpr auto Kind = InstKind::Converted.Define( - {.ir_name = "converted"}); + static constexpr auto Kind = + InstKind::Converted.Define({.ir_name = "converted"}); TypeId type_id; InstId original_id; @@ -490,8 +486,7 @@ struct Converted { // The `*` dereference operator, as in `*pointer`. struct Deref { static constexpr auto Kind = - InstKind::Deref.Define( - {.ir_name = "deref"}); + InstKind::Deref.Define({.ir_name = "deref"}); TypeId type_id; InstId pointer_id; @@ -511,9 +506,8 @@ struct ExportDecl { // Represents accessing the `type` field in a facet value, which is notionally a // pair of a type and a witness. struct FacetTypeAccess { - static constexpr auto Kind = - InstKind::FacetTypeAccess.Define( - {.ir_name = "facet_type_access"}); + static constexpr auto Kind = InstKind::FacetTypeAccess.Define( + {.ir_name = "facet_type_access"}); TypeId type_id; InstId facet_id; @@ -644,9 +638,8 @@ struct AnyImportRef { // An imported entity that is not yet been loaded. struct ImportRefUnloaded { - // No parse node: any parse node logic must use the referenced IR. static constexpr auto Kind = - InstKind::ImportRefUnloaded.Define( + InstKind::ImportRefUnloaded.Define( {.ir_name = "import_ref", .is_lowered = false}); ImportIRInstId import_ir_inst_id; @@ -655,10 +648,8 @@ struct ImportRefUnloaded { // A imported entity that is loaded, and may be used. struct ImportRefLoaded { - // No parse node: any parse node logic must use the referenced IR. - static constexpr auto Kind = - InstKind::ImportRefLoaded.Define( - {.ir_name = "import_ref", .is_lowered = false}); + static constexpr auto Kind = InstKind::ImportRefLoaded.Define( + {.ir_name = "import_ref", .is_lowered = false}); TypeId type_id; ImportIRInstId import_ir_inst_id; @@ -849,8 +840,7 @@ struct SpecificConstant { // late, and splice parts together. struct SpliceBlock { static constexpr auto Kind = - InstKind::SpliceBlock.Define( - {.ir_name = "splice_block"}); + InstKind::SpliceBlock.Define({.ir_name = "splice_block"}); TypeId type_id; InstBlockId block_id; @@ -882,8 +872,7 @@ struct StructAccess { // Initializes a dest struct with the provided elements. struct StructInit { static constexpr auto Kind = - InstKind::StructInit.Define( - {.ir_name = "struct_init"}); + InstKind::StructInit.Define({.ir_name = "struct_init"}); TypeId type_id; InstBlockId elements_id; @@ -929,10 +918,9 @@ struct StructTypeField { // A struct value. struct StructValue { - static constexpr auto Kind = - InstKind::StructValue.Define( - {.ir_name = "struct_value", - .constant_kind = InstConstantKind::Conditional}); + static constexpr auto Kind = InstKind::StructValue.Define( + {.ir_name = "struct_value", + .constant_kind = InstConstantKind::Conditional}); TypeId type_id; InstBlockId elements_id; @@ -940,9 +928,8 @@ struct StructValue { // A temporary value. struct Temporary { - // Doesn't have its own nodes, only reuses locations. - static constexpr auto Kind = InstKind::Temporary.Define( - {.ir_name = "temporary"}); + static constexpr auto Kind = + InstKind::Temporary.Define({.ir_name = "temporary"}); TypeId type_id; InstId storage_id; @@ -985,8 +972,8 @@ struct TupleIndex { // Initializes the destination tuple with the given elements. struct TupleInit { - static constexpr auto Kind = InstKind::TupleInit.Define( - {.ir_name = "tuple_init"}); + static constexpr auto Kind = + InstKind::TupleInit.Define({.ir_name = "tuple_init"}); TypeId type_id; InstBlockId elements_id; @@ -1016,10 +1003,9 @@ struct TupleType { // A tuple value. struct TupleValue { - static constexpr auto Kind = - InstKind::TupleValue.Define( - {.ir_name = "tuple_value", - .constant_kind = InstConstantKind::Conditional}); + static constexpr auto Kind = InstKind::TupleValue.Define( + {.ir_name = "tuple_value", + .constant_kind = InstConstantKind::Conditional}); TypeId type_id; InstBlockId elements_id; @@ -1095,6 +1081,12 @@ template concept HasNodeId = !std::same_as; +// HasUntypedNodeId is true if T has an associated parse node which can be any +// kind of node. +template +concept HasUntypedNodeId = + std::same_as; + // HasKindMemberAsField is true if T has a `InstKind kind` field, as opposed // to a `static constexpr InstKind::Definition Kind` member or no kind at all. template