Represent vtables as a top level SemIR construct #5472

dwblaikie · 2025-05-13T17:54:40Z

The goal was/is to reduce the overhead for vtables in generics - the previous representation/prior to this patch caused a new vtable to be created in every specific which isn't generally what we want for Carbon generics (the whole specific/generic thing is meant to avoid creating specific versions for things that can be a generic form parameterized by a specific instead of manifest as a unique entity per specific)

So this moves vtables to a top level object (like functions, classes, etc). Each dynamic class will have a vtable in this list.

Classes have a vtable_ptr instruction in them that points to the vtable.

The actual generic support hasn't been implemented in this patch, as I've been struggling with just getting this part of the migration going & wanted to get it flushed out before adding the additional complications.

It's possible more laziness when doing cross-file importing would be suitable - for instance if we only need to reference the vtable from another file, but don't need to know its individual contents, it may be beneficial for the functions in the vtable to be import_refs (or to add another layer of indirection - so it can be a single import_ref
all-or-nothing for the functions in the vtable).

…neric The goal was/is to reduce the overhead for vtables in generics - the previous representation/prior to this patch caused a new vtable to be created in every specific which isn't generally what we want for Carbon generics (the whole specific/generic thing is meant to avoid creating specific versions for things that can be a generic form parameterized by a specific instead of manifest as a unique entity per specific) So this moves vtables to a top level object (like functions, classes, etc). Each dynamic class will have a vtable in this list. Classes have a `vtable_ptr` instruction in them that points to the vtable. The actual generic support hasn't been implemented in this patch, as I've been struggling with just getting this part of the migration going & wanted to get it flushed out before adding the additional complications. It's possible more laziness when doing cross-file importing would be suitable - for instance if we only need to reference the vtable from another file, but don't need to know its individual contents, it may be beneficial for the functions in the vtable to be import_refs (or to add another layer of indirection - so it can be a single import_ref all-or-nothing for the functions in the vtable).

zygoloid · 2025-06-09T22:06:36Z

toolchain/check/class.cpp

@@ -131,22 +131,14 @@ static auto AddStructTypeFields(

 // Builds and returns a vtable for the current class. Assumes that the virtual
 // functions for the class are listed as the top element of the `vtable_stack`.
-static auto BuildVtable(Context& context, Parse::NodeId node_id,
-                        SemIR::InstId base_vtable_id,
+static auto BuildVtable(Context& context, Parse::NodeId /* node_id */,


Drop the node_id parameter if it's not needed. Seems easy to add back if that changes.

zygoloid · 2025-06-09T22:08:02Z

toolchain/check/class.cpp

@@ -155,7 +147,7 @@ static auto BuildVtable(Context& context, Parse::NodeId node_id,
      for (auto override_fn_decl_id : vtable_contents) {
        auto override_fn_decl =
            context.insts().GetAs<SemIR::FunctionDecl>(override_fn_decl_id);
-        const auto& override_fn =
+        auto& override_fn =


Can fn be const auto&? Seems nice if we can mark the base class's functions as const here.

zygoloid · 2025-06-09T22:11:58Z

toolchain/check/class.cpp

+        if (canonical_base_vtable_inst_id == SemIR::InstId::None) {
+          return SemIR::ErrorInst::InstId;
+        }


Producing an error without generating a diagnostic, when there's no error marking (eg ErrorInst) found, is unusual. If this is necessary, it'd be good to have a comment here explaining what's happening.

zygoloid · 2025-06-09T22:14:50Z

toolchain/check/class.cpp

+        AddInst<SemIR::VtablePtr>(context, node_id,
+                                  {.type_id = vptr_type_id,
+                                   .vtable_id = vtable_id,
+                                   .specific_id = SemIR::SpecificId::None});


I think this should be context.generics().GetSelfSpecific(class_info.generic_id), so that we compute a different vtable_ptr for each specialization of the class. (This is more vtables <-> generics stuff, so maybe just a TODO here for now?)

zygoloid · 2025-06-09T22:17:10Z

toolchain/check/class.cpp

+        const auto& base_vtable_ptr_inst =
+            context.insts().GetAs<SemIR::VtablePtr>(
+                canonical_base_vtable_inst_id);
+        base_vtable_id = base_vtable_ptr_inst.vtable_id;


We could also capture the specific_id here and pass it into BuildVtable for use in CheckFunctionTypeMatches. (Maybe not in this PR, though; it's probably better to handle vtables <-> generics interactions in a separate change.)

zygoloid · 2025-06-09T22:24:26Z

toolchain/check/import_ref.cpp

@@ -1710,15 +1712,15 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
                      ? GetLocalConstantInstId(resolver, import_class.adapt_id)
                      : SemIR::InstId::None;
  auto& new_class = resolver.local_classes().Get(class_id);
+  auto vtable_ptr_const_id =


Maybe add a TODO here to not load this eagerly?

zygoloid · 2025-06-09T22:31:16Z

toolchain/lower/file_context.cpp

+  for (const auto& [id, class_info] : sem_ir_->vtables().enumerate()) {
+    if (auto* vtable = BuildVtable(class_info)) {
+      vtables_.Insert(id, vtable);
+    }
+  }


With the new cross-file lowering logic, moving this from LowerDefinitions to PrepareToLower will cause vtable definitions for file A to get emitted when generating code for file B, if B uses a generic defined in A. I tried to keep this as similar to the old code as possible when adding the new cross-file stuff to avoid creating merge conflicts here.

What we should be doing here is probably: in PrepareToLower, generate declarations for vtables, and in LowerDefinitions, add initializers to convert those declarations to definitions. That's roughly what we do for variables (except that for variables we generate the declarations as a side effect of lowering the constant, rather than as a separate step in PrepareToLower).

For now, can we get away with keeping this loop in LowerDefinitions, so that we can add the cross-file stuff in a different PR, or is that no longer working? If not, moving it here with a TODO for now might work.

zygoloid · 2025-06-09T22:36:08Z

toolchain/lower/handle.cpp

@@ -333,7 +333,7 @@ auto HandleInst(FunctionContext& context, SemIR::InstId inst_id,

 auto HandleInst(FunctionContext& context, SemIR::InstId inst_id,
                SemIR::VtablePtr inst) -> void {
-  context.SetLocal(inst_id, context.GetValue(inst.vtable_id));
+  context.SetLocal(inst_id, context.GetVtable(inst.vtable_id));


Is this reachable? I'd expect that we never see a non-constant vtable_ptr instruction in lowering, and could CARBON_FATAL here instead.

zygoloid · 2025-06-09T22:39:38Z

toolchain/sem_ir/inst_namer.cpp

@@ -693,6 +709,10 @@ auto InstNamer::NamingContext::NameInst() -> void {
      AddInstName("complete_type");
      return;
    }
+    case VtablePtr::Kind: {
+      AddInstName("vtable_ptr");


Would be nice to include the class name here, when the instruction is in the constants scope.

toolchain/sem_ir/vtable.h

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

github-actions bot added the toolchain label May 13, 2025

dwblaikie force-pushed the vtable_as_top_level branch from c6792eb to adede1b Compare June 5, 2025 23:43

dwblaikie requested a review from zygoloid June 5, 2025 23:45

dwblaikie marked this pull request as ready for review June 5, 2025 23:45

dwblaikie force-pushed the vtable_as_top_level branch from adede1b to 13b8fc5 Compare June 6, 2025 00:08

zygoloid reviewed Jun 9, 2025

View reviewed changes

dwblaikie and others added 9 commits June 17, 2025 20:49

Remove unused NodeId parameter

bcda335

Make base function (being overriden) const, since we're not touching it.

255eb25

Add TODO for vtable specifics.

de07c2c

Add TODO for retrieving the specific for the base vtable

d92b6de

Add TODO comment for specific_id passing through convert

feb67c4

Replace HandleInst implementation with CARBON_FATAL

3bb9d49

Add class name to the vtable_ptr instruction name in sem_ir dump

62c1c05

Remove auto-generated local-path specific comment

6a38f1e

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

Remove checking for non-constant vtable_inst_id

967796c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Represent vtables as a top level SemIR construct #5472

Represent vtables as a top level SemIR construct #5472

dwblaikie commented May 13, 2025 •

edited

Loading

Uh oh!

zygoloid Jun 9, 2025

Uh oh!

zygoloid Jun 9, 2025

Uh oh!

zygoloid Jun 9, 2025

Uh oh!

zygoloid Jun 9, 2025

Uh oh!

zygoloid Jun 9, 2025

Uh oh!

zygoloid Jun 9, 2025

Uh oh!

zygoloid Jun 9, 2025

Uh oh!

zygoloid Jun 9, 2025

Uh oh!

zygoloid Jun 9, 2025

Uh oh!

Uh oh!

Uh oh!

Represent vtables as a top level SemIR construct #5472

Are you sure you want to change the base?

Represent vtables as a top level SemIR construct #5472

Conversation

dwblaikie commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dwblaikie commented May 13, 2025 •

edited

Loading