WIP SemIR vtables

[dwblaikie]

No description provided.

[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