-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP SemIR vtables #4732
base: trunk
Are you sure you want to change the base?
WIP SemIR vtables #4732
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1392,6 +1392,26 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver, | |||||||||||||||||||||||||
return ResolveResult::Done(resolver.local_constant_values().Get(inst_id)); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
static auto TryResolveTypedInst(ImportRefResolver& resolver, SemIR::Vtable inst, | ||||||||||||||||||||||||||
SemIR::InstId import_inst_id) -> ResolveResult { | ||||||||||||||||||||||||||
auto type_const_id = GetLocalConstantId(resolver, inst.type_id); | ||||||||||||||||||||||||||
auto virtual_functions = | ||||||||||||||||||||||||||
GetLocalInstBlockContents(resolver, inst.virtual_functions_id); | ||||||||||||||||||||||||||
if (resolver.HasNewWork()) { | ||||||||||||||||||||||||||
return ResolveResult::Retry(); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
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)); | ||||||||||||||||||||||||||
Comment on lines
+1406
to
+1412
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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):
where D2 has the correct vtable:
but D1 has this weird vtable:
Which then lead to a CHECK-fail in the commented out vtable initialization code in this PR, here:
|
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
static auto TryResolveTypedInst(ImportRefResolver& resolver, | ||||||||||||||||||||||||||
SemIR::BindAlias inst) -> ResolveResult { | ||||||||||||||||||||||||||
auto value_id = GetLocalConstantId(resolver, inst.value_id); | ||||||||||||||||||||||||||
|
@@ -1513,8 +1533,8 @@ static auto AddClassDefinition(ImportContext& context, | |||||||||||||||||||||||||
const SemIR::Class& import_class, | ||||||||||||||||||||||||||
SemIR::Class& new_class, | ||||||||||||||||||||||||||
SemIR::InstId complete_type_witness_id, | ||||||||||||||||||||||||||
SemIR::InstId base_id, SemIR::InstId adapt_id) | ||||||||||||||||||||||||||
-> void { | ||||||||||||||||||||||||||
SemIR::InstId base_id, SemIR::InstId adapt_id, | ||||||||||||||||||||||||||
SemIR::InstId vtable_id) -> void { | ||||||||||||||||||||||||||
new_class.definition_id = new_class.first_owning_decl_id; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
new_class.complete_type_witness_id = complete_type_witness_id; | ||||||||||||||||||||||||||
|
@@ -1537,6 +1557,9 @@ static auto AddClassDefinition(ImportContext& context, | |||||||||||||||||||||||||
if (import_class.adapt_id.is_valid()) { | ||||||||||||||||||||||||||
new_class.adapt_id = adapt_id; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
if (import_class.vtable_id.is_valid()) { | ||||||||||||||||||||||||||
new_class.vtable_id = vtable_id; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
static auto TryResolveTypedInst(ImportRefResolver& resolver, | ||||||||||||||||||||||||||
|
@@ -1602,6 +1625,10 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver, | |||||||||||||||||||||||||
auto adapt_id = import_class.adapt_id.is_valid() | ||||||||||||||||||||||||||
? GetLocalConstantInstId(resolver, import_class.adapt_id) | ||||||||||||||||||||||||||
: SemIR::InstId::Invalid; | ||||||||||||||||||||||||||
auto vtable_id = | ||||||||||||||||||||||||||
import_class.vtable_id.is_valid() | ||||||||||||||||||||||||||
? GetLocalConstantInstId(resolver, import_class.vtable_id) | ||||||||||||||||||||||||||
: SemIR::InstId::Invalid; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (resolver.HasNewWork()) { | ||||||||||||||||||||||||||
return ResolveResult::Retry(class_const_id); | ||||||||||||||||||||||||||
|
@@ -1625,7 +1652,7 @@ static auto TryResolveTypedInst(ImportRefResolver& resolver, | |||||||||||||||||||||||||
SemIR::WitnessType::SingletonInstId), | ||||||||||||||||||||||||||
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); | ||||||||||||||||||||||||||
complete_type_witness_id, base_id, adapt_id, vtable_id); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return ResolveResult::Done(class_const_id); | ||||||||||||||||||||||||||
|
@@ -2674,6 +2701,9 @@ static auto TryResolveInstCanonical(ImportRefResolver& resolver, | |||||||||||||||||||||||||
case CARBON_KIND(SemIR::UnboundElementType inst): { | ||||||||||||||||||||||||||
return TryResolveTypedInst(resolver, inst); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
case CARBON_KIND(SemIR::Vtable inst): { | ||||||||||||||||||||||||||
return TryResolveTypedInst(resolver, inst, inst_id); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
default: { | ||||||||||||||||||||||||||
// This instruction might have a constant value of a different kind. | ||||||||||||||||||||||||||
auto constant_inst_id = | ||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inst_id
comes fromAddInstInNoBlock
so looking it up inlocal_constant_values()
looks wrong to me. Should it belocal_insts()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
but, yeah, throwing things in the dark here a bit :/