Add a vtableDecl inst and use that in classes instead of VtablePtr (#5945)

This addresses/avoids the duplicate import of vtables.

I went through a few iterations/etc along the way and left them in the
commit
history for the PR in case any of them are useful to illustrate how I
got here,
or worth revisiting.

Essentially I ended up with a circularity in importing - importing the
class
imported the vtable_decl which imported the virtual functions - and then
pending
specifics of the virtual functions needed the self specific of the
enclosing
class which wasn't ready yet.

Adding ImportRef to the vtable_decl to break the cycle caused me trouble
when
naming the vtable_decl instructions - so I tried making the functions in
the
vtable unloaded ImportRefs instead. That worked, but meant that
importing a
class still was doing O(number of vtable entries) even if the vtable
wasn't
used.

So I revisited the lazy vtable_decl - figured out how to make the naming
work
(when building the vtable_ptr, even though the vtable_decl doesn't have
to be
loaded for the vtable_ptr, I force it to be loaded anyway, to load the
vtable so
it's usable by lowering, etc). And then I could go back to the old
non-lazy
loaded vtable entries (using some loaded ImportRefs in the cases where
we needed
them/had already adopted them).

Then thinking about the VtablePtr instruction, went back/forth on
exactly what
it needed - went from VtablePtr's member being a VtableDecl InstId, to a
ClassId, then back to a VtableId as it was before this patch.

Naming the instructions has one oddity, that the VtableDecl and
VtablePtr
instructions seem to need to add the pending name for the VtableId -
despite not
using the VtableId in their own name - should the inst namer be doing
this work
for parameters of instructions rather than requiring the inst to do it
deliberately? (or am I holding it wrong in some way?)

---------

Co-authored-by: Richard Smith <richard@metafoo.co.uk>

toolchain/check/class.cpp

@@ -145,15 +145,15 @@ static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id,
   // Get some base class/type/specific info.
   if (base_class_type) {
     auto& base_class_info = context.classes().Get(base_class_type->class_id);
-    auto base_vtable_ptr_inst_id = base_class_info.vtable_ptr_id;
-    if (base_vtable_ptr_inst_id.has_value()) {
-      LoadImportRef(context, base_vtable_ptr_inst_id);
+    auto base_vtable_decl_inst_id = base_class_info.vtable_decl_id;
+    if (base_vtable_decl_inst_id.has_value()) {
+      LoadImportRef(context, base_vtable_decl_inst_id);
       auto canonical_base_vtable_inst_id =
-          context.constant_values().GetConstantInstId(base_vtable_ptr_inst_id);
-      const auto& base_vtable_ptr_inst =
-          context.insts().GetAs<SemIR::VtablePtr>(
+          context.constant_values().GetConstantInstId(base_vtable_decl_inst_id);
+      const auto& base_vtable_decl_inst =
+          context.insts().GetAs<SemIR::VtableDecl>(
               canonical_base_vtable_inst_id);
-      base_vtable_id = base_vtable_ptr_inst.vtable_id;
+      base_vtable_id = base_vtable_decl_inst.vtable_id;
       base_class_specific_id = base_class_type->specific_id;
     }
   }
@@ -187,7 +187,6 @@ static auto BuildVtable(Context& context, Parse::ClassDefinitionId node_id,
     // TODO: Avoid quadratic search. Perhaps build a map from `NameId` to the
     // elements of the top of `vtable_stack`.
     for (auto base_vtable_entry_id : base_vtable_inst_block) {
-      LoadImportRef(context, base_vtable_entry_id);
       auto [derived_vtable_entry_id, derived_vtable_entry_const_id, fn_id,
             specific_id] =
           DecomposeVirtualFunction(context.sem_ir(), base_vtable_entry_id,
@@ -303,15 +302,9 @@ static auto CheckCompleteClassType(
   if (class_info.is_dynamic) {
     auto vtable_id = BuildVtable(context, node_id, class_id, base_class_type,
                                  vtable_contents);
-
     auto vptr_type_id = GetPointerType(context, SemIR::VtableType::TypeInstId);
-    auto generic_id = class_info.generic_id;
-    auto self_specific_id = context.generics().GetSelfSpecific(generic_id);
-    class_info.vtable_ptr_id =
-        AddInst<SemIR::VtablePtr>(context, node_id,
-                                  {.type_id = vptr_type_id,
-                                   .vtable_id = vtable_id,
-                                   .specific_id = self_specific_id});
+    class_info.vtable_decl_id = AddInst<SemIR::VtableDecl>(
+        context, node_id, {.type_id = vptr_type_id, .vtable_id = vtable_id});
   }
 
   auto struct_type_id = GetStructType(

toolchain/check/convert.cpp

@@ -165,7 +165,7 @@ static auto ConvertAggregateElement(
     ConversionTarget::Kind kind, SemIR::InstId target_id,
     SemIR::TypeInstId target_elem_type_inst, PendingBlock* target_block,
     size_t src_field_index, size_t target_field_index,
-    SemIR::InstId vtable_ptr_inst_id = SemIR::InstId::None) -> SemIR::InstId {
+    SemIR::ClassType* vtable_class_type = nullptr) -> SemIR::InstId {
   auto src_elem_type =
       context.types().GetTypeIdForTypeInstId(src_elem_type_inst);
   auto target_elem_type =
@@ -193,7 +193,7 @@ static auto ConvertAggregateElement(
   target.init_id = MakeElementAccessInst<TargetAccessInstT>(
       context, loc_id, target_id, target_elem_type, *target_block,
       target_field_index);
-  return Convert(context, loc_id, src_elem_id, target, vtable_ptr_inst_id);
+  return Convert(context, loc_id, src_elem_id, target, vtable_class_type);
 }
 
 // Performs a conversion from a tuple to an array type. This function only
@@ -390,7 +390,7 @@ template <typename TargetAccessInstT>
 static auto ConvertStructToStructOrClass(
     Context& context, SemIR::StructType src_type, SemIR::StructType dest_type,
     SemIR::InstId value_id, ConversionTarget target,
-    SemIR::InstId vtable_ptr_inst_id = SemIR::InstId::None) -> SemIR::InstId {
+    SemIR::ClassType* vtable_class_type = nullptr) -> SemIR::InstId {
   static_assert(std::is_same_v<SemIR::ClassElementAccess, TargetAccessInstT> ||
                 std::is_same_v<SemIR::StructAccess, TargetAccessInstT>);
   constexpr bool ToClass =
@@ -480,11 +480,22 @@ static auto ConvertStructToStructOrClass(
                                              {.type_id = vptr_type_id,
                                               .base_id = target.init_id,
                                               .index = SemIR::ElementIndex(i)});
-      auto init_id =
-          AddInst<SemIR::InitializeFrom>(context, value_loc_id,
-                                         {.type_id = vptr_type_id,
-                                          .src_id = vtable_ptr_inst_id,
-                                          .dest_id = dest_id});
+      auto vtable_decl_id =
+          context.classes().Get(vtable_class_type->class_id).vtable_decl_id;
+      LoadImportRef(context, vtable_decl_id);
+      auto canonical_vtable_decl_id =
+          context.constant_values().GetConstantInstId(vtable_decl_id);
+      auto vtable_ptr_id = AddInst<SemIR::VtablePtr>(
+          context, value_loc_id,
+          {.type_id = GetPointerType(context, SemIR::VtableType::TypeInstId),
+           .vtable_id = context.insts()
+                            .GetAs<SemIR::VtableDecl>(canonical_vtable_decl_id)
+                            .vtable_id,
+           .specific_id = vtable_class_type->specific_id});
+      auto init_id = AddInst<SemIR::InitializeFrom>(context, value_loc_id,
+                                                    {.type_id = vptr_type_id,
+                                                     .src_id = vtable_ptr_id,
+                                                     .dest_id = dest_id});
       new_block.Set(i, init_id);
       continue;
     }
@@ -526,7 +537,7 @@ static auto ConvertStructToStructOrClass(
             context, value_loc_id, value_id, src_field.type_inst_id,
             literal_elems, inner_kind, target.init_id, dest_field.type_inst_id,
             target.init_block, src_field_index,
-            src_field_index + dest_vptr_offset, vtable_ptr_inst_id);
+            src_field_index + dest_vptr_offset, vtable_class_type);
     if (init_id == SemIR::ErrorInst::InstId) {
       return SemIR::ErrorInst::InstId;
     }
@@ -568,10 +579,11 @@ static auto ConvertStructToStruct(Context& context, SemIR::StructType src_type,
 // Performs a conversion from a struct to a class type. This function only
 // converts the type, and does not perform a final conversion to the requested
 // expression category.
-static auto ConvertStructToClass(
-    Context& context, SemIR::StructType src_type, SemIR::ClassType dest_type,
-    SemIR::InstId value_id, ConversionTarget target,
-    SemIR::InstId dest_vtable_ptr_inst_id = SemIR::InstId::None)
+static auto ConvertStructToClass(Context& context, SemIR::StructType src_type,
+                                 SemIR::ClassType dest_type,
+                                 SemIR::InstId value_id,
+                                 ConversionTarget target,
+                                 SemIR::ClassType* vtable_class_type)
     -> SemIR::InstId {
   PendingBlock target_block(&context);
   auto& dest_class_info = context.classes().Get(dest_type.class_id);
@@ -598,24 +610,9 @@ static auto ConvertStructToClass(
         SemIR::LocId(value_id), {.type_id = target.type_id});
   }
 
-  if (!dest_vtable_ptr_inst_id.has_value()) {
-    dest_vtable_ptr_inst_id = dest_class_info.vtable_ptr_id;
-    if (dest_type.specific_id.has_value() &&
-        dest_vtable_ptr_inst_id.has_value()) {
-      LoadImportRef(context, dest_vtable_ptr_inst_id);
-      dest_vtable_ptr_inst_id = context.constant_values().GetInstId(
-          GetConstantValueInSpecific(context.sem_ir(), dest_type.specific_id,
-                                     dest_vtable_ptr_inst_id));
-    }
-  }
-
-  if (dest_vtable_ptr_inst_id.has_value()) {
-    LoadImportRef(context, dest_vtable_ptr_inst_id);
-  }
-
   auto result_id = ConvertStructToStructOrClass<SemIR::ClassElementAccess>(
       context, src_type, dest_struct_type, value_id, target,
-      dest_vtable_ptr_inst_id);
+      vtable_class_type ? vtable_class_type : &dest_type);
 
   if (need_temporary) {
     target_block.InsertHere();
@@ -799,8 +796,8 @@ static auto DiagnoseConversionFailureToConstraintValue(
 
 static auto PerformBuiltinConversion(
     Context& context, SemIR::LocId loc_id, SemIR::InstId value_id,
-    ConversionTarget target,
-    SemIR::InstId vtable_ptr_inst_id = SemIR::InstId::None) -> SemIR::InstId {
+    ConversionTarget target, SemIR::ClassType* vtable_class_type = nullptr)
+    -> SemIR::InstId {
   auto& sem_ir = context.sem_ir();
   auto value = sem_ir.insts().Get(value_id);
   auto value_type_id = value.type_id();
@@ -974,7 +971,7 @@ static auto PerformBuiltinConversion(
                .adapt_id.has_value()) {
         return ConvertStructToClass(context, *src_struct_type,
                                     *target_class_type, value_id, target,
-                                    vtable_ptr_inst_id);
+                                    vtable_class_type);
       }
     }
 
@@ -1201,7 +1198,7 @@ auto PerformAction(Context& context, SemIR::LocId loc_id,
 }
 
 auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
-             ConversionTarget target, SemIR::InstId vtable_ptr_inst_id)
+             ConversionTarget target, SemIR::ClassType* vtable_class_type)
     -> SemIR::InstId {
   auto& sem_ir = context.sem_ir();
   auto orig_expr_id = expr_id;
@@ -1266,7 +1263,7 @@ auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
 
   // Check whether any builtin conversion applies.
   expr_id = PerformBuiltinConversion(context, loc_id, expr_id, target,
-                                     vtable_ptr_inst_id);
+                                     vtable_class_type);
   if (expr_id == SemIR::ErrorInst::InstId) {
     return expr_id;
   }

toolchain/check/convert.h

@@ -67,8 +67,7 @@ struct ConversionTarget {
 // type.
 auto Convert(Context& context, SemIR::LocId loc_id, SemIR::InstId expr_id,
              ConversionTarget target,
-             SemIR::InstId vtable_ptr_inst_id = SemIR::InstId::None)
-    -> SemIR::InstId;
+             SemIR::ClassType* vtable_class_type = nullptr) -> SemIR::InstId;
 
 // Performs initialization of `target_id` from `value_id`. Returns the
 // possibly-converted initializing expression, which should be assigned to the

toolchain/check/import_ref.cpp

@@ -1649,7 +1649,7 @@ static auto AddClassDefinition(ImportContext& context,
                                SemIR::Class& new_class,
                                SemIR::InstId complete_type_witness_id,
                                SemIR::InstId base_id, SemIR::InstId adapt_id,
-                               SemIR::InstId vtable_ptr_id) -> void {
+                               SemIR::InstId vtable_decl_id) -> void {
   new_class.definition_id = new_class.first_owning_decl_id;
 
   new_class.complete_type_witness_id = complete_type_witness_id;
@@ -1671,8 +1671,8 @@ static auto AddClassDefinition(ImportContext& context,
   if (import_class.adapt_id.has_value()) {
     new_class.adapt_id = adapt_id;
   }
-  if (import_class.vtable_ptr_id.has_value()) {
-    new_class.vtable_ptr_id = vtable_ptr_id;
+  if (import_class.vtable_decl_id.has_value()) {
+    new_class.vtable_decl_id = vtable_decl_id;
   }
 }
 
@@ -1740,11 +1740,11 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
   auto adapt_id = import_class.adapt_id.has_value()
                       ? GetLocalConstantInstId(resolver, import_class.adapt_id)
                       : SemIR::InstId::None;
-  auto& new_class = resolver.local_classes().Get(class_id);
-  auto vtable_ptr_const_id =
-      import_class.vtable_ptr_id.has_value()
-          ? AddImportRef(resolver, import_class.vtable_ptr_id)
+  auto vtable_decl_id =
+      import_class.vtable_decl_id.has_value()
+          ? AddImportRef(resolver, import_class.vtable_decl_id)
           : SemIR::InstId::None;
+  auto& new_class = resolver.local_classes().Get(class_id);
 
   if (resolver.HasNewWork()) {
     return ResolveResult::Retry(class_const_id, new_class.first_decl_id());
@@ -1770,7 +1770,7 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
         import_class.complete_type_witness_id, complete_type_witness_const_id);
     AddClassDefinition(resolver, import_class, new_class,
                        complete_type_witness_id, base_id, adapt_id,
-                       vtable_ptr_const_id);
+                       vtable_decl_id);
   }
 
   return ResolveResult::Done(class_const_id, new_class.first_decl_id());
@@ -2043,18 +2043,14 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
 }
 
 static auto TryResolveTypedInst(ImportRefResolver& resolver,
-                                SemIR::VtablePtr inst,
+                                SemIR::VtableDecl inst,
                                 SemIR::ConstantId /*vtable_const_id*/)
     -> ResolveResult {
   const auto& import_vtable = resolver.import_vtables().Get(inst.vtable_id);
-
   auto class_const_id =
       GetLocalConstantId(resolver, resolver.import_classes()
                                        .Get(import_vtable.class_id)
                                        .first_owning_decl_id);
-
-  auto specific_data = GetLocalSpecificData(resolver, inst.specific_id);
-
   auto class_const_inst = resolver.local_insts().Get(
       resolver.local_constant_values().GetInstId(class_const_id));
 
@@ -2111,12 +2107,35 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
         .virtual_functions_id =
             resolver.local_inst_blocks().Add(lazy_virtual_functions)}});
 
-  return ResolveAsDeduplicated<SemIR::VtablePtr>(
+  return ResolveAsDeduplicated<SemIR::VtableDecl>(
       resolver, {.type_id = GetPointerType(resolver.local_context(),
                                            SemIR::VtableType::TypeInstId),
-                 .vtable_id = new_vtable_id,
-                 .specific_id = GetOrAddLocalSpecific(
-                     resolver, inst.specific_id, specific_data)});
+                 .vtable_id = new_vtable_id});
+}
+
+static auto TryResolveTypedInst(ImportRefResolver& resolver,
+                                SemIR::VtablePtr inst,
+                                SemIR::ConstantId /*vtable_const_id*/)
+    -> ResolveResult {
+  auto specific_data = GetLocalSpecificData(resolver, inst.specific_id);
+
+  auto vtable_const_id = GetLocalConstantId(
+      resolver, resolver.import_classes()
+                    .Get(resolver.import_vtables().Get(inst.vtable_id).class_id)
+                    .vtable_decl_id);
+  auto vtable_const_inst = resolver.local_insts().Get(
+      resolver.local_constant_values().GetInstId(vtable_const_id));
+  if (resolver.HasNewWork()) {
+    return ResolveResult::Retry();
+  }
+
+  return ResolveAsDeduplicated<SemIR::VtablePtr>(
+      resolver,
+      {.type_id = GetPointerType(resolver.local_context(),
+                                 SemIR::VtableType::TypeInstId),
+       .vtable_id = vtable_const_inst.As<SemIR::VtableDecl>().vtable_id,
+       .specific_id =
+           GetOrAddLocalSpecific(resolver, inst.specific_id, specific_data)});
 }
 
 static auto TryResolveTypedInst(ImportRefResolver& resolver,
@@ -3227,6 +3246,9 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver,
     case CARBON_KIND(SemIR::VarStorage inst): {
       return TryResolveTypedInst(resolver, inst, inst_id);
     }
+    case CARBON_KIND(SemIR::VtableDecl inst): {
+      return TryResolveTypedInst(resolver, inst, const_id);
+    }
     case CARBON_KIND(SemIR::VtablePtr inst): {
       return TryResolveTypedInst(resolver, inst, const_id);
     }

toolchain/lower/testdata/class/virtual.carbon

@@ -537,7 +537,6 @@ fn Make() {
 // CHECK:STDOUT: source_filename = "generic_base.carbon"
 // CHECK:STDOUT:
 // CHECK:STDOUT: @"_CDerived.Main.$vtable" = unnamed_addr constant [1 x i32] [i32 trunc (i64 sub (i64 ptrtoint (ptr @_CF.Base.Main.4d2ffa01ebfb7a1d to i64), i64 ptrtoint (ptr @"_CDerived.Main.$vtable" to i64)) to i32)]
-// CHECK:STDOUT: @"_CBase.Main.$vtable.4d2ffa01ebfb7a1d" = unnamed_addr constant [1 x i32] [i32 trunc (i64 sub (i64 ptrtoint (ptr @_CF.Base.Main.4d2ffa01ebfb7a1d to i64), i64 ptrtoint (ptr @"_CBase.Main.$vtable.4d2ffa01ebfb7a1d" to i64)) to i32)]
 // CHECK:STDOUT:
 // CHECK:STDOUT: define void @_CMake.Main() !dbg !4 {
 // CHECK:STDOUT: entry:

toolchain/sem_ir/class.h

@@ -63,7 +63,7 @@ struct ClassFields {
 
   // The virtual function table. `None` if the class has no (direct or
   // inherited) virtual functions.
-  InstId vtable_ptr_id = InstId::None;
+  InstId vtable_decl_id = InstId::None;
 
   auto PrintClassFields(llvm::raw_ostream& out) const -> void {
     out << "self_type_id: " << self_type_id << ", inheritance_kind: ";
@@ -82,7 +82,7 @@ struct ClassFields {
         << ", body_block_id: " << body_block_id << ", adapt_id: " << adapt_id
         << ", base_id: " << base_id
         << ", complete_type_witness_id: " << complete_type_witness_id
-        << ", vtable_ptr_id: " << vtable_ptr_id << "}";
+        << ", vtable_decl_id: " << vtable_decl_id << "}";
   }
 };
 

toolchain/sem_ir/expr_info.cpp

@@ -184,6 +184,7 @@ auto GetExprCategory(const File& file, InstId inst_id) -> ExprCategory {
       }
 
       case VtablePtr::Kind:
+      case VtableDecl::Kind:
         return ExprCategory::EphemeralRef;
 
       case CARBON_KIND(ClassElementAccess inst): {

toolchain/sem_ir/formatter.cpp

@@ -347,10 +347,10 @@ auto Formatter::FormatClass(ClassId id) -> void {
     out_ << "complete_type_witness = ";
     FormatName(class_info.complete_type_witness_id);
     out_ << "\n";
-    if (class_info.vtable_ptr_id.has_value()) {
+    if (class_info.vtable_decl_id.has_value()) {
       Indent();
-      out_ << "vtable_ptr = ";
-      FormatName(class_info.vtable_ptr_id);
+      out_ << "vtable_decl = ";
+      FormatName(class_info.vtable_decl_id);
       out_ << "\n";
     }
 

toolchain/sem_ir/inst_kind.def

@@ -146,6 +146,7 @@ CARBON_SEM_IR_INST_KIND(ValueParam)
 CARBON_SEM_IR_INST_KIND(ValueParamPattern)
 CARBON_SEM_IR_INST_KIND(VarPattern)
 CARBON_SEM_IR_INST_KIND(VarStorage)
+CARBON_SEM_IR_INST_KIND(VtableDecl)
 CARBON_SEM_IR_INST_KIND(VtableType)
 CARBON_SEM_IR_INST_KIND(VtablePtr)
 CARBON_SEM_IR_INST_KIND(WhereExpr)

toolchain/sem_ir/inst_namer.cpp

@@ -783,16 +783,23 @@ auto InstNamer::NamingContext::NameInst() -> void {
       AddInstName("complete_type");
       return;
     }
-    case CARBON_KIND(VtablePtr inst): {
+    case CARBON_KIND(VtableDecl inst): {
       const auto& vtable = sem_ir().vtables().Get(inst.vtable_id);
+      inst_namer_->MaybePushEntity(inst.vtable_id);
       if (inst_namer_->GetScopeFor(vtable.class_id) == scope_id_) {
-        inst_namer_->MaybePushEntity(inst.vtable_id);
-        AddInstName("vtable_ptr");
+        inst_namer_->MaybePushEntity(vtable.class_id);
+        AddInstName("vtable_decl");
       } else {
-        AddEntityNameAndMaybePush(inst.vtable_id, "_ptr");
+        AddEntityNameAndMaybePush(vtable.class_id, ".vtable_decl");
       }
       return;
     }
+    case CARBON_KIND(VtablePtr inst): {
+      const auto& vtable = sem_ir().vtables().Get(inst.vtable_id);
+      inst_namer_->MaybePushEntity(inst.vtable_id);
+      AddEntityNameAndMaybePush(vtable.class_id, ".vtable_ptr");
+      return;
+    }
     case ConstType::Kind: {
       // TODO: Can we figure out the name of the type argument?
       AddInstName("const");

toolchain/sem_ir/typed_insts.h

@@ -1889,6 +1889,15 @@ struct VtablePtr {
   SpecificId specific_id;
 };
 
+struct VtableDecl {
+  static constexpr auto Kind = InstKind::VtableDecl.Define<Parse::NodeId>(
+      {.ir_name = "vtable_decl",
+       .constant_kind = InstConstantKind::Always,
+       .is_lowered = false});
+  TypeId type_id;
+  VtableId vtable_id;
+};
+
 // An `expr where requirements` expression.
 struct WhereExpr {
   static constexpr auto Kind = InstKind::WhereExpr.Define<Parse::WhereExprId>(