Add SemIR Vtable instruction and usage

[dwblaikie]

Add a Vtable typed inst with a type_id (of the type this vtable applies to) and list of virtual function decls (or import refs to function object constants).

This doesn't add lowering/emission of the vtable, or usage when initializing objects of the type.

Some questions in case they're interesting to discuss:

• is it right/worth having the type_id in the vtable? (probably makes it easier to emit - using the type to get the class name to figure out the mangled name for the vtable) perhaps it should be a ClassId?
• I'm thinking the logic in CheckCompleteClassType could be the place we handle diagnostics for mismatched keywords (virtual/abstract for a function that's already virtual/abstract, maybe checking for non-virtual functions with the same name in a base class, or derived class functions without impl, etc) - but we could move some of that to the moment we walk the function decl, and record our findings in the function decl (record the base function it overrides, or the index of the vtable to slot to use when building the vtable at the end of the class)
• the Vtable typed inst has constant_kind = InstConstantKind::Always and is_lowered = false, I think I added that in to workaround/address some failures in lowering. And seems correct for this intermediate step - I'll add lowering in a follow-up patch. But the constant_kind - what should this be? We can just say all vtables are of VtableType (in which case the Always constant kind sounds right to me) or we could have them introduce a type with each virtual function as a named member, even?

[danakj]

inst_id comes from AddInstInNoBlock so looking it up in local_constant_values() looks wrong to me. Should it be local_insts()?

[dwblaikie]

I tried something like this, but didn't have any luck changing the small test case outcome (& autoupdate still crashes, which may or may not be related/the same crash - mostly ignoring that until I can get a small import-and-derive test case to work)

diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp
index 0a89777bf..b23c7ce81 100644
--- a/toolchain/check/import_ref.cpp
+++ b/toolchain/check/import_ref.cpp
@@ -1393,7 +1393,8 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver,
 }

 static auto TryResolveTypedInst(ImportRefResolver& resolver, SemIR::Vtable inst,
-                                SemIR::InstId import_inst_id) -> ResolveResult {
+                                SemIR::InstId /*import_inst_id*/)
+    -> ResolveResult {
   auto type_const_id = GetLocalConstantId(resolver, inst.type_id);
   auto virtual_functions =
       GetLocalInstBlockContents(resolver, inst.virtual_functions_id);
@@ -1403,13 +1404,10 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver, SemIR::Vtable inst,

   auto virtual_functions_id = GetLocalCanonicalInstBlockId(
       resolver, inst.virtual_functions_id, virtual_functions);
-  auto inst_id = resolver.local_context().AddInstInNoBlock(
-      resolver.local_context().MakeImportedLocAndInst<SemIR::Vtable>(
-          AddImportIRInst(resolver, import_inst_id),
-          {.type_id =
-               resolver.local_context().GetTypeIdForTypeConstant(type_const_id),
-           .virtual_functions_id = virtual_functions_id}));
-  return ResolveResult::Done(resolver.local_constant_values().Get(inst_id));
+  return ResolveAs<SemIR::Vtable>(
+      resolver, {.type_id = resolver.local_context().GetTypeIdForTypeConstant(
+                     type_const_id),
+                 .virtual_functions_id = virtual_functions_id});
 }

but, yeah, throwing things in the dark here a bit :/

[zygoloid]

We generally only add a new instruction when importing declaration-like things where the declaration and its constant value might be separate, it might be found by name lookup, it might be redeclared, that sort of thing. If you only want to import a constant value for the vtable (which I think is what we want here), you can directly produce a constant value instead of adding an instruction and then getting its constant value:

Suggested change

	auto inst_id = resolver.local_context().AddInstInNoBlock(
	resolver.local_context().MakeImportedLocAndInst<SemIR::Vtable>(
	AddImportIRInst(resolver, import_inst_id),
	{.type_id =
	resolver.local_context().GetTypeIdForTypeConstant(type_const_id),
	.virtual_functions_id = virtual_functions_id}));
	return ResolveResult::Done(resolver.local_constant_values().Get(inst_id));
	return ResolveAs<SemIR::Vtable>(
	resolver,
	{.type_id =
	resolver.local_context().GetTypeIdForTypeConstant(type_const_id),
	.virtual_functions_id = virtual_functions_id}));

[dwblaikie]

Ah, right - I guess I tried something like this to start, encountered problems, then went down the rabbit hole of ResolveResult, etc, trying to find something that works.

So stepping back (based on the suggested edit you provided) to this simpler implementation, I should describe the problem I encounter (oh, right, I did describe it over here: #4671 (reply in thread) - sorry about the bifurcated discussion, repeating that here):

package Modifiers;
base class B1 { virtual fn H(); }

import Modifiers;
class D1 { extend base: Modifiers.B1; }
base class B2 { virtual fn H(); }
class D2 { extend base: B2; }

where D2 has the correct vtable:

%H.decl: %H.type.2 = fn_decl @H.2 [template = constants.%H.2] {} {}
 ...
%.loc4_29: <vtable> = vtable (@B2.%H.decl) [template = constants.%.2]

but D1 has this weird vtable:

 %H.1: %H.type.1 = struct_value () [template]
 ...
 %.loc2_39: <vtable> = vtable (constants.%H.1) [template = constants.%.1]

Which then lead to a CHECK-fail in the commented out vtable initialization code in this PR, here:

       auto base_vtable_inst_block = context.inst_blocks().Get(
           context.insts()
               .GetAs<SemIR::Vtable>(base_class_info->vtable_id)
               .virtual_functions_id);
       for (auto fn_decl_id : base_vtable_inst_block) {
───────> auto fn_decl = context.insts().GetAs<SemIR::FunctionDecl>(fn_decl_id);

CHECK failure at ./toolchain/sem_ir/inst.h:182: Is<TypedInst>(): Casting inst {kind: StructValue, arg0: inst_block_empty, type: type(inst22)} to wrong kind FunctionDecl

[zygoloid]

Thanks, looks good. Only a few tiny things.

[jonmeow]

Just a couple drive-by comments, I was looking out of curiosity on the approach.

[josh11b]

Why pass inst_id if it isn't going to be used?

[dwblaikie]

Ah, thanks for the catch - removed the unused parameter in #4832