Address the LocIdAndInst::ReusingLoc TODO (#4211)

The TODO for switching to ReusingLoc (previously Untyped) had been there
for a while, so I'm trying to address it here. The intent had been to be
clearer about when the construction is validated, particularly so that
we aren't accidentally accepting an incorrect NodeId. Note, this does
fix an incorrect use of InvalidNodeId where NoLoc should've been called.

Since this changes the semantics of when `Parse::NodeId` is helpful in
`typed_nodes.h`, I'm doing a pass to either refine or switch to
`Parse::InvalidNodeId` where it compiles. I think most remaining
`Parse::NodeId` examples are things we _should_ be able to refine with a
little more work (versus before where `Parse::NodeId` also indicated
`LocId` construction might be used).

I'm also changing context.h to use `requires` that match what
`LocIdAndInst` has, I think it makes the diagnostics a little better.
And note I do add an overload for `ImportIRInstId`, also matching
`LocIdAndInst`, and widely used for import refs.

toolchain/check/check.cpp

@@ -284,7 +284,7 @@ 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.AddInst<SemIR::ImportDecl>(
+        import_decl_id = context.AddInstReusingLoc<SemIR::ImportDecl>(
             import_ir_inst_id, {.package_id = SemIR::NameId::ForIdentifier(
                                     api_imports_entry.first)});
         package_id = api_imports_entry.first;

toolchain/check/context.h

@@ -67,20 +67,38 @@ class Context {
   // Adds an instruction to the current block, returning the produced ID.
   auto AddInst(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId;
 
-  // Convenience for AddInst on specific instruction types.
-  template <typename InstT, typename LocT>
-  auto AddInst(LocT loc_id, InstT inst) -> SemIR::InstId {
-    return AddInst(SemIR::LocIdAndInst(loc_id, inst));
+  // Convenience for AddInst with typed nodes.
+  template <typename InstT>
+    requires(SemIR::Internal::HasNodeId<InstT>)
+  auto AddInst(decltype(InstT::Kind)::TypedNodeId node_id, InstT inst)
+      -> SemIR::InstId {
+    return AddInst(SemIR::LocIdAndInst(node_id, inst));
+  }
+
+  // Convenience for AddInst when reusing a location, which any instruction can
+  // do.
+  template <typename InstT>
+  auto AddInstReusingLoc(SemIR::LocId loc_id, InstT inst) -> SemIR::InstId {
+    return AddInst(SemIR::LocIdAndInst::ReusingLoc<InstT>(loc_id, inst));
   }
 
   // Adds an instruction in no block, returning the produced ID. Should be used
   // rarely.
   auto AddInstInNoBlock(SemIR::LocIdAndInst loc_id_and_inst) -> SemIR::InstId;
 
-  // Convenience for AddInstInNoBlock on specific instruction types.
-  template <typename InstT, typename LocT>
-  auto AddInstInNoBlock(LocT loc_id, InstT inst) -> SemIR::InstId {
-    return AddInstInNoBlock(SemIR::LocIdAndInst(loc_id, inst));
+  // Convenience for AddInstInNoBlock with typed nodes.
+  template <typename InstT>
+    requires(SemIR::Internal::HasNodeId<InstT>)
+  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 <typename InstT>
+  auto AddInstInNoBlock(SemIR::ImportIRInstId import_ir_inst_id, InstT inst)
+      -> SemIR::InstId {
+    return AddInstInNoBlock(SemIR::LocIdAndInst(import_ir_inst_id, inst));
   }
 
   // Adds an instruction to the current block, returning the produced ID. The
@@ -99,9 +117,11 @@ class Context {
 
   // Pushes a parse tree node onto the stack, storing the SemIR::Inst as the
   // result. Only valid if the LocId is for a NodeId.
-  template <typename InstT, typename LocT>
-  auto AddInstAndPush(LocT loc_id, InstT inst) -> void {
-    SemIR::LocIdAndInst arg(loc_id, inst);
+  template <typename InstT>
+    requires(SemIR::Internal::HasNodeId<InstT>)
+  auto AddInstAndPush(decltype(InstT::Kind)::TypedNodeId node_id, InstT inst)
+      -> void {
+    SemIR::LocIdAndInst arg(node_id, inst);
     auto inst_id = AddInst(arg);
     node_stack_.Push(arg.loc_id.node_id(), inst_id);
   }

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.AddInst<SemIR::Temporary>(sem_ir.insts().GetLocId(init_id),
-                                             {.type_id = init.type_id(),
-                                              .storage_id = return_slot_id,
-                                              .init_id = init_id});
+    return context.AddInstReusingLoc<SemIR::Temporary>(
+        sem_ir.insts().GetLocId(init_id), {.type_id = init.type_id(),
+                                           .storage_id = return_slot_id,
+                                           .init_id = init_id});
   }
 
   if (discarded) {
@@ -122,11 +122,12 @@ 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.AddInst<SemIR::TemporaryStorage>(
+  auto temporary_id = context.AddInstReusingLoc<SemIR::TemporaryStorage>(
       loc_id, {.type_id = init.type_id()});
-  return context.AddInst<SemIR::Temporary>(loc_id, {.type_id = init.type_id(),
-                                                    .storage_id = temporary_id,
-                                                    .init_id = init_id});
+  return context.AddInstReusingLoc<SemIR::Temporary>(
+      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
@@ -150,14 +151,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 AddInst<SemIR::IntLiteral>(
+    auto index_id = block.template AddInstReusingLoc<SemIR::IntLiteral>(
         loc_id,
         {.type_id = context.GetBuiltinType(SemIR::BuiltinInstKind::IntType),
          .int_id = context.ints().Add(llvm::APInt(32, i))});
-    return block.template AddInst<AccessInstT>(
+    return block.template AddInstReusingLoc<AccessInstT>(
         loc_id, {elem_type_id, aggregate_id, index_id});
   } else {
-    return block.template AddInst<AccessInstT>(
+    return block.template AddInstReusingLoc<AccessInstT>(
         loc_id, {elem_type_id, aggregate_id, SemIR::ElementIndex(i)});
   }
 }
@@ -256,7 +257,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->AddInst<SemIR::TemporaryStorage>(
+    return_slot_id = target_block->AddInstReusingLoc<SemIR::TemporaryStorage>(
         value_loc_id, {.type_id = target.type_id});
   }
 
@@ -283,7 +284,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.AddInst<SemIR::ArrayInit>(
+  return context.AddInstReusingLoc<SemIR::ArrayInit>(
       value_loc_id, {.type_id = target.type_id,
                      .inits_id = sem_ir.inst_blocks().Add(inits),
                      .dest_id = return_slot_id});
@@ -363,12 +364,12 @@ static auto ConvertTupleToTuple(Context& context, SemIR::TupleType src_type,
 
   if (is_init) {
     target.init_block->InsertHere();
-    return context.AddInst<SemIR::TupleInit>(value_loc_id,
-                                             {.type_id = target.type_id,
-                                              .elements_id = new_block.id(),
-                                              .dest_id = target.init_id});
+    return context.AddInstReusingLoc<SemIR::TupleInit>(
+        value_loc_id, {.type_id = target.type_id,
+                       .elements_id = new_block.id(),
+                       .dest_id = target.init_id});
   } else {
-    return context.AddInst<SemIR::TupleValue>(
+    return context.AddInstReusingLoc<SemIR::TupleValue>(
         value_loc_id,
         {.type_id = target.type_id, .elements_id = new_block.id()});
   }
@@ -497,18 +498,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.AddInst<SemIR::ClassInit>(value_loc_id,
-                                             {.type_id = target.type_id,
-                                              .elements_id = new_block.id(),
-                                              .dest_id = target.init_id});
+    return context.AddInstReusingLoc<SemIR::ClassInit>(
+        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.AddInst<SemIR::StructInit>(value_loc_id,
-                                              {.type_id = target.type_id,
-                                               .elements_id = new_block.id(),
-                                               .dest_id = target.init_id});
+    return context.AddInstReusingLoc<SemIR::StructInit>(
+        value_loc_id, {.type_id = target.type_id,
+                       .elements_id = new_block.id(),
+                       .dest_id = target.init_id});
   } else {
-    return context.AddInst<SemIR::StructValue>(
+    return context.AddInstReusingLoc<SemIR::StructValue>(
         value_loc_id,
         {.type_id = target.type_id, .elements_id = new_block.id()});
   }
@@ -555,7 +556,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.AddInst<SemIR::TemporaryStorage>(
+    target.init_id = target_block.AddInstReusingLoc<SemIR::TemporaryStorage>(
         context.insts().GetLocId(value_id), {.type_id = target.type_id});
   }
 
@@ -564,7 +565,7 @@ static auto ConvertStructToClass(Context& context, SemIR::StructType src_type,
 
   if (need_temporary) {
     target_block.InsertHere();
-    result_id = context.AddInst<SemIR::Temporary>(
+    result_id = context.AddInstReusingLoc<SemIR::Temporary>(
         context.insts().GetLocId(value_id), {.type_id = target.type_id,
                                              .storage_id = target.init_id,
                                              .init_id = result_id});
@@ -622,7 +623,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<SemIR::BaseDecl>(base_id);
-    value_id = context.AddInst<SemIR::ClassElementAccess>(
+    value_id = context.AddInstReusingLoc<SemIR::ClassElementAccess>(
         loc_id, {.type_id = base_decl.base_type_id,
                  .base_id = value_id,
                  .index = base_decl.index});
@@ -637,14 +638,14 @@ static auto ConvertDerivedPointerToBasePointer(
     const InheritancePath& path) -> SemIR::InstId {
   // Form `*p`.
   ptr_id = ConvertToValueExpr(context, ptr_id);
-  auto ref_id = context.AddInst<SemIR::Deref>(
+  auto ref_id = context.AddInstReusingLoc<SemIR::Deref>(
       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.AddInst<SemIR::AddrOf>(
+  return context.AddInstReusingLoc<SemIR::AddrOf>(
       loc_id, {.type_id = dest_ptr_type_id, .lvalue_id = ref_id});
 }
 
@@ -743,7 +744,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.AddInst<SemIR::ValueOfInitializer>(
+        return context.AddInstReusingLoc<SemIR::ValueOfInitializer>(
             loc_id, {.type_id = value_type_id, .init_id = value_id});
       }
     }
@@ -764,7 +765,7 @@ static auto PerformBuiltinConversion(Context& context, SemIR::LocId loc_id,
             ConversionTarget{.kind = ConversionTarget::Value,
                              .type_id = value_type_id});
       }
-      return context.AddInst<SemIR::AsCompatible>(
+      return context.AddInstReusingLoc<SemIR::AsCompatible>(
           loc_id, {.type_id = target.type_id, .source_id = value_id});
     }
   }
@@ -870,7 +871,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<SemIR::InterfaceType>(value_type_id)) {
-      return context.AddInst<SemIR::FacetTypeAccess>(
+      return context.AddInstReusingLoc<SemIR::FacetTypeAccess>(
           loc_id, {.type_id = target.type_id, .facet_id = value_id});
     }
   }
@@ -981,10 +982,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.AddInst<SemIR::Converted>(loc_id, {.type_id = target.type_id,
-                                                   .original_id = orig_expr_id,
-                                                   .result_id = expr_id});
+    expr_id = context.AddInstReusingLoc<SemIR::Converted>(
+        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
@@ -1034,7 +1035,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.AddInst<SemIR::BindValue>(
+      expr_id = context.AddInstReusingLoc<SemIR::BindValue>(
           context.insts().GetLocId(expr_id),
           {.type_id = expr.type_id(), .value_id = expr_id});
       // We now have a value expression.
@@ -1053,7 +1054,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.AddInst<SemIR::InitializeFrom>(
+      expr_id = context.AddInstReusingLoc<SemIR::InitializeFrom>(
           loc_id, {.type_id = target.type_id,
                    .src_id = expr_id,
                    .dest_id = target.init_id});
@@ -1162,7 +1163,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.AddInst<SemIR::AddrOf>(
+    self_or_addr_id = context.AddInstReusingLoc<SemIR::AddrOf>(
         loc_id, {.type_id = context.GetPointerType(self.type_id()),
                  .lvalue_id = self_or_addr_id});
   }

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.AddInst<SemIR::BindAlias>(
+  auto alias_id = context.AddInstReusingLoc<SemIR::BindAlias>(
       name_context.loc_id, {.type_id = alias_type_id,
                             .entity_name_id = entity_name_id,
                             .value_id = alias_value_id});

toolchain/check/handle_interface.cpp

@@ -173,10 +173,11 @@ auto HandleParseNode(Context& context,
         {.name_id = SemIR::NameId::SelfType,
          .parent_scope_id = interface_info.scope_id,
          .bind_index = context.scope_stack().AddCompileTimeBinding()});
-    interface_info.self_param_id = context.AddInst<SemIR::BindSymbolicName>(
-        SemIR::LocId::Invalid, {.type_id = self_type_id,
-                                .entity_name_id = entity_name_id,
-                                .value_id = SemIR::InstId::Invalid});
+    interface_info.self_param_id =
+        context.AddInst(SemIR::LocIdAndInst::NoLoc<SemIR::BindSymbolicName>(
+            {.type_id = self_type_id,
+             .entity_name_id = entity_name_id,
+             .value_id = SemIR::InstId::Invalid}));
     context.scope_stack().PushCompileTimeBinding(interface_info.self_param_id);
     context.name_scopes().AddRequiredName(interface_info.scope_id,
                                           SemIR::NameId::SelfType,

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.AddInst<SemIR::AssociatedEntity>(
+  return context.AddInstReusingLoc<SemIR::AssociatedEntity>(
       context.insts().GetLocId(decl_id),
       {.type_id = type_id, .index = index, .decl_id = decl_id});
 }

toolchain/check/pending_block.h

@@ -41,8 +41,9 @@ class PendingBlock {
   };
 
   template <typename InstT>
-  auto AddInst(SemIR::LocId loc_id, InstT inst) -> SemIR::InstId {
-    auto inst_id = context_.AddInstInNoBlock(loc_id, inst);
+  auto AddInstReusingLoc(SemIR::LocId loc_id, InstT inst) -> SemIR::InstId {
+    auto inst_id = context_.AddInstInNoBlock(
+        SemIR::LocIdAndInst::ReusingLoc(loc_id, inst));
     insts_.push_back(inst_id);
     return inst_id;
   }
@@ -66,12 +67,10 @@ class PendingBlock {
       // 1) The block is empty. Replace `target_id` with an empty splice
       // pointing at `value_id`.
       context_.ReplaceLocIdAndInstBeforeConstantUse(
-          target_id,
-          SemIR::LocIdAndInst(
-              value.loc_id,
-              SemIR::SpliceBlock{.type_id = value.inst.type_id(),
-                                 .block_id = SemIR::InstBlockId::Empty,
-                                 .result_id = value_id}));
+          target_id, SemIR::LocIdAndInst::ReusingLoc<SemIR::SpliceBlock>(
+                         value.loc_id, {.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.
@@ -80,11 +79,10 @@ class PendingBlock {
       // 3) Anything else: splice it into the IR, replacing `target_id`.
       context_.ReplaceLocIdAndInstBeforeConstantUse(
           target_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}));
+          SemIR::LocIdAndInst::ReusingLoc<SemIR::SpliceBlock>(
+              value.loc_id, {.type_id = value.inst.type_id(),
+                             .block_id = context_.inst_blocks().Add(insts_),
+                             .result_id = value_id}));
     }
 
     // Prepare to stash more pending instructions.

toolchain/sem_ir/inst.h

@@ -305,11 +305,16 @@ 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.
   template <typename InstT>
   static auto NoLoc(InstT inst) -> LocIdAndInst {
     return LocIdAndInst(LocId::Invalid, inst, /*is_untyped=*/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 <typename InstT>
   static auto ReusingLoc(LocId loc_id, InstT inst) -> LocIdAndInst {
     return LocIdAndInst(loc_id, inst, /*is_untyped=*/true);
@@ -321,18 +326,11 @@ struct LocIdAndInst {
   LocIdAndInst(decltype(InstT::Kind)::TypedNodeId node_id, InstT inst)
       : loc_id(node_id), inst(inst) {}
 
-  // If TypedNodeId is Parse::NodeId, allow construction with a LocId.
-  // TODO: This is somewhat historical due to fetching the NodeId from insts()
-  // for things like Temporary; should we require Untyped in these cases?
-  template <typename InstT>
-    requires(std::same_as<typename decltype(InstT::Kind)::TypedNodeId,
-                          Parse::NodeId>)
-  LocIdAndInst(LocId loc_id, InstT inst) : loc_id(loc_id), inst(inst) {}
-
   // Imports can pass an ImportIRInstId instead of another location.
   template <typename InstT>
   LocIdAndInst(ImportIRInstId import_ir_inst_id, InstT inst)
       : loc_id(import_ir_inst_id), inst(inst) {}
+
   LocId loc_id;
   Inst inst;
 

toolchain/sem_ir/typed_insts.h

@@ -55,9 +55,10 @@ struct AdaptDecl {
 
 // The `&` address-of operator, as in `&lvalue`.
 struct AddrOf {
-  // TODO: Make Parse::NodeId more specific.
-  static constexpr auto Kind = InstKind::AddrOf.Define<Parse::NodeId>(
-      {.ir_name = "addr_of", .constant_kind = InstConstantKind::Conditional});
+  static constexpr auto Kind =
+      InstKind::AddrOf.Define<Parse::PrefixOperatorAmpId>(
+          {.ir_name = "addr_of",
+           .constant_kind = InstConstantKind::Conditional});
 
   TypeId type_id;
   InstId lvalue_id;
@@ -76,9 +77,8 @@ struct AddrPattern {
 
 // An array indexing operation, such as `array[index]`.
 struct ArrayIndex {
-  // TODO: Make Parse::NodeId more specific.
-  static constexpr auto Kind =
-      InstKind::ArrayIndex.Define<Parse::NodeId>({.ir_name = "array_index"});
+  static constexpr auto Kind = InstKind::ArrayIndex.Define<Parse::IndexExprId>(
+      {.ir_name = "array_index"});
 
   TypeId type_id;
   InstId array_id;
@@ -136,9 +136,8 @@ struct AnyAggregateValue {
 // expression. `inits_id` contains one initializer per array element.
 // `dest_id` is the destination array object for the initialization.
 struct ArrayInit {
-  // TODO: Make Parse::NodeId more specific.
-  static constexpr auto Kind =
-      InstKind::ArrayInit.Define<Parse::NodeId>({.ir_name = "array_init"});
+  static constexpr auto Kind = InstKind::ArrayInit.Define<Parse::InvalidNodeId>(
+      {.ir_name = "array_init"});
 
   TypeId type_id;
   InstBlockId inits_id;
@@ -159,8 +158,9 @@ struct ArrayType {
 
 // Perform a no-op conversion to a compatible type.
 struct AsCompatible {
-  static constexpr auto Kind = InstKind::AsCompatible.Define<Parse::NodeId>(
-      {.ir_name = "as_compatible"});
+  static constexpr auto Kind =
+      InstKind::AsCompatible.Define<Parse::InvalidNodeId>(
+          {.ir_name = "as_compatible"});
 
   TypeId type_id;
   InstId source_id;
@@ -182,7 +182,7 @@ struct Assign {
 // An associated constant declaration in an interface, such as `let T:! type;`.
 struct AssociatedConstantDecl {
   static constexpr auto Kind =
-      InstKind::AssociatedConstantDecl.Define<Parse::NodeId>(
+      InstKind::AssociatedConstantDecl.Define<Parse::LetDeclId>(
           {.ir_name = "assoc_const_decl", .is_lowered = false});
 
   TypeId type_id;
@@ -194,8 +194,10 @@ 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<Parse::NodeId>(
-      {.ir_name = "assoc_entity", .constant_kind = InstConstantKind::Always});
+  static constexpr auto Kind =
+      InstKind::AssociatedEntity.Define<Parse::InvalidNodeId>(
+          {.ir_name = "assoc_entity",
+           .constant_kind = InstConstantKind::Always});
 
   // The type of the associated entity. This is an AssociatedEntityType.
   TypeId type_id;
@@ -312,6 +314,7 @@ struct BlockArg {
 
 // A literal bool value, `true` or `false`.
 struct BoolLiteral {
+  // TODO: Make Parse::NodeId more specific.
   static constexpr auto Kind = InstKind::BoolLiteral.Define<Parse::NodeId>(
       {.ir_name = "bool_literal", .constant_kind = InstConstantKind::Always});
 
@@ -441,9 +444,8 @@ struct ClassElementAccess {
 
 // Initializes a class object at dest_id with the contents of elements_id.
 struct ClassInit {
-  // TODO: Make Parse::NodeId more specific.
-  static constexpr auto Kind =
-      InstKind::ClassInit.Define<Parse::NodeId>({.ir_name = "class_init"});
+  static constexpr auto Kind = InstKind::ClassInit.Define<Parse::InvalidNodeId>(
+      {.ir_name = "class_init"});
 
   TypeId type_id;
   InstBlockId elements_id;
@@ -452,7 +454,7 @@ struct ClassInit {
 
 // The type for a class, either non-generic or specific.
 struct ClassType {
-  static constexpr auto Kind = InstKind::ClassType.Define<Parse::NodeId>(
+  static constexpr auto Kind = InstKind::ClassType.Define<Parse::InvalidNodeId>(
       {.ir_name = "class_type",
        .is_type = InstIsType::Always,
        .constant_kind = InstConstantKind::Always});
@@ -477,8 +479,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<Parse::NodeId>({.ir_name = "converted"});
+  static constexpr auto Kind = InstKind::Converted.Define<Parse::InvalidNodeId>(
+      {.ir_name = "converted"});
 
   TypeId type_id;
   InstId original_id;
@@ -487,9 +489,9 @@ struct Converted {
 
 // The `*` dereference operator, as in `*pointer`.
 struct Deref {
-  // TODO: Make Parse::NodeId more specific.
   static constexpr auto Kind =
-      InstKind::Deref.Define<Parse::NodeId>({.ir_name = "deref"});
+      InstKind::Deref.Define<Parse::AnyPointerDeferenceExprId>(
+          {.ir_name = "deref"});
 
   TypeId type_id;
   InstId pointer_id;
@@ -498,7 +500,7 @@ struct Deref {
 // An `export bind_name` declaration.
 struct ExportDecl {
   static constexpr auto Kind =
-      InstKind::ExportDecl.Define<Parse::NodeId>({.ir_name = "export"});
+      InstKind::ExportDecl.Define<Parse::ExportDeclId>({.ir_name = "export"});
 
   TypeId type_id;
   EntityNameId entity_name_id;
@@ -509,8 +511,9 @@ 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<Parse::NodeId>(
-      {.ir_name = "facet_type_access"});
+  static constexpr auto Kind =
+      InstKind::FacetTypeAccess.Define<Parse::InvalidNodeId>(
+          {.ir_name = "facet_type_access"});
 
   TypeId type_id;
   InstId facet_id;
@@ -541,7 +544,7 @@ struct FloatLiteral {
 
 // A floating point type.
 struct FloatType {
-  static constexpr auto Kind = InstKind::FloatType.Define<Parse::NodeId>(
+  static constexpr auto Kind = InstKind::FloatType.Define<Parse::InvalidNodeId>(
       {.ir_name = "float_type",
        .is_type = InstIsType::Always,
        .constant_kind = InstConstantKind::Conditional});
@@ -666,7 +669,9 @@ struct ImportRefLoaded {
 // `src_id`, by performing a final copy from source to destination, for types
 // whose initialization is not in-place.
 struct InitializeFrom {
-  // TODO: Make Parse::NodeId more specific.
+  // Note this Parse::NodeId is unused. InitializeFrom is only constructed by
+  // reusing locations.
+  // TODO: Figure out if there's a better way to handle this case.
   static constexpr auto Kind = InstKind::InitializeFrom.Define<Parse::NodeId>(
       {.ir_name = "initialize_from"});
 
@@ -692,10 +697,11 @@ struct InterfaceDecl {
 
 // The type for an interface, either non-generic or specific.
 struct InterfaceType {
-  static constexpr auto Kind = InstKind::InterfaceType.Define<Parse::NodeId>(
-      {.ir_name = "interface_type",
-       .is_type = InstIsType::Always,
-       .constant_kind = InstConstantKind::Always});
+  static constexpr auto Kind =
+      InstKind::InterfaceType.Define<Parse::InvalidNodeId>(
+          {.ir_name = "interface_type",
+           .is_type = InstIsType::Always,
+           .constant_kind = InstConstantKind::Always});
 
   TypeId type_id;
   InterfaceId interface_id;
@@ -740,7 +746,7 @@ struct IntLiteral {
 
 // An integer type.
 struct IntType {
-  static constexpr auto Kind = InstKind::IntType.Define<Parse::NodeId>(
+  static constexpr auto Kind = InstKind::IntType.Define<Parse::InvalidNodeId>(
       {.ir_name = "int_type",
        .is_type = InstIsType::Always,
        .constant_kind = InstConstantKind::Conditional});
@@ -790,11 +796,11 @@ struct Param {
 // Modifies a pointee type to be a pointer. This is tracking the `*` in
 // `x: i32*`, where `pointee_id` is `i32` and `type_id` is `type`.
 struct PointerType {
-  // TODO: Make Parse::NodeId more specific.
-  static constexpr auto Kind = InstKind::PointerType.Define<Parse::NodeId>(
-      {.ir_name = "ptr_type",
-       .is_type = InstIsType::Always,
-       .constant_kind = InstConstantKind::Conditional});
+  static constexpr auto Kind =
+      InstKind::PointerType.Define<Parse::PostfixOperatorStarId>(
+          {.ir_name = "ptr_type",
+           .is_type = InstIsType::Always,
+           .constant_kind = InstConstantKind::Conditional});
 
   TypeId type_id;
   TypeId pointee_id;
@@ -829,6 +835,7 @@ struct ReturnExpr {
 // Consider merging an `SpecificConstant` + `NameRef` into a new form of
 // instruction in order to give a more compact representation.
 struct SpecificConstant {
+  // TODO: Can we make Parse::NodeId more specific?
   static constexpr auto Kind = InstKind::SpecificConstant.Define<Parse::NodeId>(
       {.ir_name = "specific_constant", .is_lowered = false});
 
@@ -842,9 +849,9 @@ struct SpecificConstant {
 // constructing from aggregates we may figure out which conversions are required
 // late, and splice parts together.
 struct SpliceBlock {
-  // TODO: Can we make Parse::NodeId more specific?
   static constexpr auto Kind =
-      InstKind::SpliceBlock.Define<Parse::NodeId>({.ir_name = "splice_block"});
+      InstKind::SpliceBlock.Define<Parse::InvalidNodeId>(
+          {.ir_name = "splice_block"});
 
   TypeId type_id;
   InstBlockId block_id;
@@ -875,9 +882,9 @@ struct StructAccess {
 
 // Initializes a dest struct with the provided elements.
 struct StructInit {
-  // TODO: Make Parse::NodeId more specific.
   static constexpr auto Kind =
-      InstKind::StructInit.Define<Parse::NodeId>({.ir_name = "struct_init"});
+      InstKind::StructInit.Define<Parse::InvalidNodeId>(
+          {.ir_name = "struct_init"});
 
   TypeId type_id;
   InstBlockId elements_id;
@@ -896,12 +903,11 @@ struct StructLiteral {
 
 // The type of a struct.
 struct StructType {
-  // TODO: Make this more specific. It can be one of: ClassDefinitionId,
-  // StructLiteralId, StructTypeLiteralId
-  static constexpr auto Kind = InstKind::StructType.Define<Parse::NodeId>(
-      {.ir_name = "struct_type",
-       .is_type = InstIsType::Always,
-       .constant_kind = InstConstantKind::Conditional});
+  static constexpr auto Kind =
+      InstKind::StructType.Define<Parse::StructTypeLiteralId>(
+          {.ir_name = "struct_type",
+           .is_type = InstIsType::Always,
+           .constant_kind = InstConstantKind::Conditional});
 
   TypeId type_id;
   InstBlockId fields_id;
@@ -924,10 +930,10 @@ struct StructTypeField {
 
 // A struct value.
 struct StructValue {
-  // TODO: Make Parse::NodeId more specific.
-  static constexpr auto Kind = InstKind::StructValue.Define<Parse::NodeId>(
-      {.ir_name = "struct_value",
-       .constant_kind = InstConstantKind::Conditional});
+  static constexpr auto Kind =
+      InstKind::StructValue.Define<Parse::InvalidNodeId>(
+          {.ir_name = "struct_value",
+           .constant_kind = InstConstantKind::Conditional});
 
   TypeId type_id;
   InstBlockId elements_id;
@@ -935,9 +941,9 @@ struct StructValue {
 
 // A temporary value.
 struct Temporary {
-  // TODO: Make Parse::NodeId more specific.
-  static constexpr auto Kind =
-      InstKind::Temporary.Define<Parse::NodeId>({.ir_name = "temporary"});
+  // Doesn't have its own nodes, only reuses locations.
+  static constexpr auto Kind = InstKind::Temporary.Define<Parse::InvalidNodeId>(
+      {.ir_name = "temporary"});
 
   TypeId type_id;
   InstId storage_id;
@@ -979,9 +985,8 @@ struct TupleIndex {
 
 // Initializes the destination tuple with the given elements.
 struct TupleInit {
-  // TODO: Make Parse::NodeId more specific.
-  static constexpr auto Kind =
-      InstKind::TupleInit.Define<Parse::NodeId>({.ir_name = "tuple_init"});
+  static constexpr auto Kind = InstKind::TupleInit.Define<Parse::InvalidNodeId>(
+      {.ir_name = "tuple_init"});
 
   TypeId type_id;
   InstBlockId elements_id;
@@ -1000,8 +1005,7 @@ struct TupleLiteral {
 
 // The type of a tuple.
 struct TupleType {
-  // TODO: Make Parse::NodeId more specific.
-  static constexpr auto Kind = InstKind::TupleType.Define<Parse::NodeId>(
+  static constexpr auto Kind = InstKind::TupleType.Define<Parse::InvalidNodeId>(
       {.ir_name = "tuple_type",
        .is_type = InstIsType::Always,
        .constant_kind = InstConstantKind::Conditional});
@@ -1012,10 +1016,10 @@ struct TupleType {
 
 // A tuple value.
 struct TupleValue {
-  // TODO: Make Parse::NodeId more specific.
-  static constexpr auto Kind = InstKind::TupleValue.Define<Parse::NodeId>(
-      {.ir_name = "tuple_value",
-       .constant_kind = InstConstantKind::Conditional});
+  static constexpr auto Kind =
+      InstKind::TupleValue.Define<Parse::InvalidNodeId>(
+          {.ir_name = "tuple_value",
+           .constant_kind = InstConstantKind::Conditional});
 
   TypeId type_id;
   InstBlockId elements_id;