Skip to content

Commit

Permalink
lazy field index (carbon-language#4514)
Browse files Browse the repository at this point in the history 
We considered a couple of other options for this:
* carbon-language#4515 Keep the
`ElementIndex` numbering vptr-ignorant, and do +1 offsets as needed -
seems subtle/easy to miss
* carbon-language#4517 Always have a
zeroth element in the object representation, make it zero-size in the
case of no-vptr - @zygoloid was concerned this would add overhead
especially to stateless objects used in type-trait-like things.

But currently moving forward with this direction - of initializing field
indexes with an invalid value until the end of the class definition,
then assigning field indexes during construction of the class's object
representation struct type. This direction might reinforce/help avoid
premature access to the object representation before the class is
complete, and give a single place where class layout is done (at class
completion) if we want to add more options there, such as class layout
optimizations, etc.

This patch still has problems with object initialization (that carbon-language#4515
does not have/does address) but does address normal `obj.member` access
correctly.

---------

Co-authored-by: Richard Smith <[email protected]>
  • Loading branch information
@dwblaikie @zygoloid @bricknerb
2 people authored and bricknerb committed Nov 28, 2024
1 parent cf71ad1 commit cda0e4c
Show file tree
Hide file tree
Showing 9 changed files with 358 additions and 118 deletions.
9 changes: 9 additions & 0 deletions toolchain/check/context.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,15 @@ auto Context::ReplaceInstBeforeConstantUse(SemIR::InstId inst_id,
FinishInst(inst_id, inst);
}

auto Context::ReplaceInstPreservingConstantValue(SemIR::InstId inst_id,
SemIR::Inst inst) -> void {
auto old_const_id = sem_ir().constant_values().Get(inst_id);
sem_ir().insts().Set(inst_id, inst);
CARBON_VLOG("ReplaceInst: {0} -> {1}\n", inst_id, inst);
auto new_const_id = TryEvalInst(*this, inst_id, inst);
CARBON_CHECK(old_const_id == new_const_id);
}

auto Context::DiagnoseDuplicateName(SemIRLoc dup_def, SemIRLoc prev_def)
-> void {
CARBON_DIAGNOSTIC(NameDeclDuplicate, Error,
Expand Down
15 changes: 13 additions & 2 deletions toolchain/check/context.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,11 @@ class Context {
auto ReplaceInstBeforeConstantUse(SemIR::InstId inst_id, SemIR::Inst inst)
-> void;

// Replaces the instruction `inst_id` with `inst`, not affecting location.
// The instruction is required to not change its constant value.
auto ReplaceInstPreservingConstantValue(SemIR::InstId inst_id,
SemIR::Inst inst) -> void;

// Sets only the parse node of an instruction. This is only used when setting
// the parse node of an imported namespace. Versus
// ReplaceInstBeforeConstantUse, it is safe to use after the namespace is used
Expand Down Expand Up @@ -492,6 +497,10 @@ class Context {
return struct_type_fields_stack_;
}

auto field_decls_stack() -> ArrayStack<SemIR::InstId>& {
return field_decls_stack_;
}

auto decl_name_stack() -> DeclNameStack& { return decl_name_stack_; }

auto decl_introducer_state_stack() -> DeclIntroducerStateStack& {
Expand Down Expand Up @@ -648,10 +657,12 @@ class Context {
// arguments.
InstBlockStack args_type_info_stack_;

// The stack of StructTypeFields for in-progress StructTypeLiterals and Class
// object representations.
// The stack of StructTypeFields for in-progress StructTypeLiterals.
ArrayStack<SemIR::StructTypeField> struct_type_fields_stack_;

// The stack of FieldDecls for in-progress Class definitions.
ArrayStack<SemIR::InstId> field_decls_stack_;

// The stack used for qualified declaration name construction.
DeclNameStack decl_name_stack_;

Expand Down
12 changes: 4 additions & 8 deletions toolchain/check/handle_binding_pattern.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,15 +128,11 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id,
auto field_type_id = context.GetUnboundElementType(
class_info.self_type_id, cast_type_id);
auto field_id = context.AddInst<SemIR::FieldDecl>(
binding_id,
{.type_id = field_type_id,
.name_id = name_id,
.index = SemIR::ElementIndex(
context.struct_type_fields_stack().PeekArray().size())});
binding_id, {.type_id = field_type_id,
.name_id = name_id,
.index = SemIR::ElementIndex::Invalid});
context.field_decls_stack().AppendToTop(field_id);

// Add a corresponding field to the object representation of the class.
context.struct_type_fields_stack().AppendToTop(
{.name_id = name_id, .type_id = cast_type_id});
context.node_stack().Push(node_id, field_id);
break;
}
Expand Down
89 changes: 57 additions & 32 deletions toolchain/check/handle_class.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ auto HandleParseNode(Context& context, Parse::ClassDefinitionStartId node_id)

context.inst_block_stack().Push();
context.node_stack().Push(node_id, class_id);
context.struct_type_fields_stack().PushArray();
context.field_decls_stack().PushArray();

// TODO: Handle the case where there's control flow in the class body. For
// example:
Expand Down Expand Up @@ -512,7 +512,7 @@ auto HandleParseNode(Context& context, Parse::BaseDeclId node_id) -> bool {
return true;
}

if (!context.struct_type_fields_stack().PeekArray().empty()) {
if (!context.field_decls_stack().PeekArray().empty()) {
// TODO: Add note that includes the first field location as an example.
CARBON_DIAGNOSTIC(
BaseDeclAfterFieldDecl, Error,
Expand All @@ -523,28 +523,23 @@ auto HandleParseNode(Context& context, Parse::BaseDeclId node_id) -> bool {

auto base_info = CheckBaseType(context, base_type_node_id, base_type_expr_id);

// TODO: Should we diagnose if there are already any fields?

// The `base` value in the class scope has an unbound element type. Instance
// binding will be performed when it's found by name lookup into an instance.
auto field_type_id =
context.GetUnboundElementType(class_info.self_type_id, base_info.type_id);
class_info.base_id = context.AddInst<SemIR::BaseDecl>(
node_id, {.type_id = field_type_id,
.base_type_id = base_info.type_id,
.index = SemIR::ElementIndex(
context.struct_type_fields_stack().PeekArray().size())});
.index = SemIR::ElementIndex::Invalid});

if (base_info.type_id != SemIR::TypeId::Error) {
auto base_class_info = context.classes().Get(
context.types().GetAs<SemIR::ClassType>(base_info.type_id).class_id);
class_info.is_dynamic |= base_class_info.is_dynamic;
}

// Add a corresponding field to the object representation of the class.
// TODO: Consider whether we want to use `partial T` here.
// TODO: Should we diagnose if there are already any fields?
context.struct_type_fields_stack().AppendToTop(
{.name_id = SemIR::NameId::Base, .type_id = base_info.type_id});

// Bind the name `base` in the class to the base field.
context.decl_name_stack().AddNameOrDiagnoseDuplicate(
context.decl_name_stack().MakeUnqualifiedName(node_id,
Expand All @@ -567,8 +562,7 @@ auto HandleParseNode(Context& context, Parse::BaseDeclId node_id) -> bool {
// returns a corresponding complete type witness instruction.
static auto CheckCompleteAdapterClassType(Context& context,
Parse::NodeId node_id,
SemIR::ClassId class_id,
SemIR::StructTypeFieldsId fields_id)
SemIR::ClassId class_id)
-> SemIR::InstId {
const auto& class_info = context.classes().Get(class_id);
if (class_info.base_id.is_valid()) {
Expand All @@ -581,17 +575,14 @@ static auto CheckCompleteAdapterClassType(Context& context,
return SemIR::InstId::BuiltinErrorInst;
}

if (auto fields = context.struct_type_fields().Get(fields_id);
!fields.empty()) {
auto [first_field_inst_id, _] = context.LookupNameInExactScope(
node_id, fields.front().name_id, class_info.scope_id,
context.name_scopes().Get(class_info.scope_id));
auto field_decls = context.field_decls_stack().PeekArray();
if (!field_decls.empty()) {
CARBON_DIAGNOSTIC(AdaptWithFields, Error, "adapter with fields");
CARBON_DIAGNOSTIC(AdaptWithFieldHere, Note,
"first field declaration is here");
context.emitter()
.Build(class_info.adapt_id, AdaptWithFields)
.Note(first_field_inst_id, AdaptWithFieldHere)
.Note(field_decls.front(), AdaptWithFieldHere)
.Emit();
return SemIR::InstId::BuiltinErrorInst;
}
Expand Down Expand Up @@ -637,46 +628,79 @@ static auto CheckCompleteAdapterClassType(Context& context,
{.type_id = context.GetBuiltinType(SemIR::BuiltinInstKind::WitnessType),
.object_repr_id = adapted_type_id});
}
static auto AddStructTypeFields(
Context& context,
llvm::SmallVector<SemIR::StructTypeField>& struct_type_fields)
-> SemIR::StructTypeFieldsId {
for (auto field_decl_id : context.field_decls_stack().PeekArray()) {
auto field_decl = context.insts().GetAs<SemIR::FieldDecl>(field_decl_id);
field_decl.index =
SemIR::ElementIndex{static_cast<int>(struct_type_fields.size())};
context.ReplaceInstPreservingConstantValue(field_decl_id, field_decl);
if (field_decl.type_id == SemIR::TypeId::Error) {
struct_type_fields.push_back(
{.name_id = field_decl.name_id, .type_id = SemIR::TypeId::Error});
continue;
}
auto unbound_element_type =
context.sem_ir().types().GetAs<SemIR::UnboundElementType>(
field_decl.type_id);
struct_type_fields.push_back(
{.name_id = field_decl.name_id,
.type_id = unbound_element_type.element_type_id});
}
auto fields_id =
context.struct_type_fields().AddCanonical(struct_type_fields);
return fields_id;
}

// Checks that the specified finished class definition is valid and builds and
// returns a corresponding complete type witness instruction.
static auto CheckCompleteClassType(Context& context, Parse::NodeId node_id,
SemIR::ClassId class_id) -> SemIR::InstId {
auto& class_info = context.classes().Get(class_id);
if (class_info.adapt_id.is_valid()) {
auto fields_id = context.struct_type_fields().AddCanonical(
context.struct_type_fields_stack().PeekArray());
context.struct_type_fields_stack().PopArray();

return CheckCompleteAdapterClassType(context, node_id, class_id, fields_id);
return CheckCompleteAdapterClassType(context, node_id, class_id);
}

bool defining_vtable_ptr = class_info.is_dynamic;
bool defining_vptr = class_info.is_dynamic;
if (class_info.base_id.is_valid()) {
auto base_info = context.insts().GetAs<SemIR::BaseDecl>(class_info.base_id);
// TODO: If the base class is template dependent, we will need to decide
// whether to add a vptr as part of instantiation.
if (auto* base_class_info = TryGetAsClass(context, base_info.base_type_id);
base_class_info && base_class_info->is_dynamic) {
defining_vtable_ptr = false;
defining_vptr = false;
}
}

if (defining_vtable_ptr) {
context.struct_type_fields_stack().PrependToTop(
auto field_decls = context.field_decls_stack().PeekArray();
llvm::SmallVector<SemIR::StructTypeField> struct_type_fields;
struct_type_fields.reserve(defining_vptr + class_info.base_id.is_valid() +
field_decls.size());
if (defining_vptr) {
struct_type_fields.push_back(
{.name_id = SemIR::NameId::Vptr,
.type_id = context.GetPointerType(
context.GetBuiltinType(SemIR::BuiltinInstKind::VtableType))});
}

auto fields_id = context.struct_type_fields().AddCanonical(
context.struct_type_fields_stack().PeekArray());
context.struct_type_fields_stack().PopArray();
if (class_info.base_id.is_valid()) {
auto base_decl = context.insts().GetAs<SemIR::BaseDecl>(class_info.base_id);
base_decl.index =
SemIR::ElementIndex{static_cast<int>(struct_type_fields.size())};
context.ReplaceInstPreservingConstantValue(class_info.base_id, base_decl);
struct_type_fields.push_back(
{.name_id = SemIR::NameId::Base,
.type_id = context.insts()
.GetAs<SemIR::BaseDecl>(class_info.base_id)
.base_type_id});
}

return context.AddInst<SemIR::CompleteTypeWitness>(
node_id,
{.type_id = context.GetBuiltinType(SemIR::BuiltinInstKind::WitnessType),
.object_repr_id = context.GetStructType(fields_id)});
.object_repr_id = context.GetStructType(
AddStructTypeFields(context, struct_type_fields))});
}

auto HandleParseNode(Context& context, Parse::ClassDefinitionId node_id)
Expand All @@ -691,6 +715,7 @@ auto HandleParseNode(Context& context, Parse::ClassDefinitionId node_id)
class_info.complete_type_witness_id = complete_type_witness_id;

context.inst_block_stack().Pop();
context.field_decls_stack().PopArray();

FinishGenericDefinition(context, class_info.generic_id);

Expand Down
4 changes: 2 additions & 2 deletions toolchain/check/handle_function.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,8 @@ static auto BuildFunctionDecl(Context& context,
parent_scope_inst) {
if (auto class_decl = parent_scope_inst->TryAs<SemIR::ClassDecl>()) {
auto& class_info = context.classes().Get(class_decl->class_id);
CARBON_CHECK(virtual_modifier != SemIR::Function::VirtualModifier::Impl ||
class_info.is_dynamic);
// TODO: If this is an `impl` function, check there's a matching base
// function that's impl or virtual.
class_info.is_dynamic = true;
}
}
Expand Down
14 changes: 7 additions & 7 deletions toolchain/check/testdata/class/fail_adapt_with_subobjects.carbon
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ class AdaptWithBaseAndFields {
// CHECK:STDOUT: %.loc10_12.2: type = converted %int.make_type_signed, %.loc10_12.1 [template = constants.%i32]
// CHECK:STDOUT: adapt_decl %i32
// CHECK:STDOUT: %Base.ref: type = name_ref Base, file.%Base.decl [template = constants.%Base]
// CHECK:STDOUT: %.loc15: %.5 = base_decl %Base, element0 [template]
// CHECK:STDOUT: %.loc15: %.5 = base_decl %Base, element<invalid> [template]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = constants.%AdaptWithBase
Expand Down Expand Up @@ -171,7 +171,7 @@ class AdaptWithBaseAndFields {
// CHECK:STDOUT: %int.make_type_signed.loc13: init type = call constants.%Int(%.loc13_10.1) [template = constants.%i32]
// CHECK:STDOUT: %.loc13_10.2: type = value_of_initializer %int.make_type_signed.loc13 [template = constants.%i32]
// CHECK:STDOUT: %.loc13_10.3: type = converted %int.make_type_signed.loc13, %.loc13_10.2 [template = constants.%i32]
// CHECK:STDOUT: %.loc13_8: %.2 = field_decl n, element0 [template]
// CHECK:STDOUT: %.loc13_8: %.2 = field_decl n, element<invalid> [template]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = constants.%AdaptWithField
Expand All @@ -188,17 +188,17 @@ class AdaptWithBaseAndFields {
// CHECK:STDOUT: %int.make_type_signed.loc25: init type = call constants.%Int(%.loc25_10.1) [template = constants.%i32]
// CHECK:STDOUT: %.loc25_10.2: type = value_of_initializer %int.make_type_signed.loc25 [template = constants.%i32]
// CHECK:STDOUT: %.loc25_10.3: type = converted %int.make_type_signed.loc25, %.loc25_10.2 [template = constants.%i32]
// CHECK:STDOUT: %.loc25_8: %.3 = field_decl a, element0 [template]
// CHECK:STDOUT: %.loc25_8: %.3 = field_decl a, element<invalid> [template]
// CHECK:STDOUT: %.loc26_10.1: Core.IntLiteral = int_value 32 [template = constants.%.1]
// CHECK:STDOUT: %int.make_type_signed.loc26: init type = call constants.%Int(%.loc26_10.1) [template = constants.%i32]
// CHECK:STDOUT: %.loc26_10.2: type = value_of_initializer %int.make_type_signed.loc26 [template = constants.%i32]
// CHECK:STDOUT: %.loc26_10.3: type = converted %int.make_type_signed.loc26, %.loc26_10.2 [template = constants.%i32]
// CHECK:STDOUT: %.loc26_8: %.3 = field_decl b, element1 [template]
// CHECK:STDOUT: %.loc26_8: %.3 = field_decl b, element<invalid> [template]
// CHECK:STDOUT: %.loc27_10.1: Core.IntLiteral = int_value 32 [template = constants.%.1]
// CHECK:STDOUT: %int.make_type_signed.loc27: init type = call constants.%Int(%.loc27_10.1) [template = constants.%i32]
// CHECK:STDOUT: %.loc27_10.2: type = value_of_initializer %int.make_type_signed.loc27 [template = constants.%i32]
// CHECK:STDOUT: %.loc27_10.3: type = converted %int.make_type_signed.loc27, %.loc27_10.2 [template = constants.%i32]
// CHECK:STDOUT: %.loc27_8: %.3 = field_decl c, element2 [template]
// CHECK:STDOUT: %.loc27_8: %.3 = field_decl c, element<invalid> [template]
// CHECK:STDOUT:
// CHECK:STDOUT: !members:
// CHECK:STDOUT: .Self = constants.%AdaptWithFields
Expand Down Expand Up @@ -250,12 +250,12 @@ class AdaptWithBaseAndFields {
// CHECK:STDOUT:
// CHECK:STDOUT: class @AdaptWithBaseAndFields {
// CHECK:STDOUT: %Base.ref: type = name_ref Base, file.%Base.decl [template = constants.%Base]
// CHECK:STDOUT: %.loc7: %.4 = base_decl %Base, element0 [template]
// CHECK:STDOUT: %.loc7: %.4 = base_decl %Base, element<invalid> [template]
// CHECK:STDOUT: %.loc8_10.1: Core.IntLiteral = int_value 32 [template = constants.%.5]
// CHECK:STDOUT: %int.make_type_signed: init type = call constants.%Int(%.loc8_10.1) [template = constants.%i32]
// CHECK:STDOUT: %.loc8_10.2: type = value_of_initializer %int.make_type_signed [template = constants.%i32]
// CHECK:STDOUT: %.loc8_10.3: type = converted %int.make_type_signed, %.loc8_10.2 [template = constants.%i32]
// CHECK:STDOUT: %.loc8_8: %.6 = field_decl n, element1 [template]
// CHECK:STDOUT: %.loc8_8: %.6 = field_decl n, element<invalid> [template]
// CHECK:STDOUT: %.loc15_10: %.1 = struct_literal ()
// CHECK:STDOUT: %.loc15_11: type = converted %.loc15_10, constants.%.1 [template = constants.%.1]
// CHECK:STDOUT: adapt_decl %.1
Expand Down
Loading

0 comments on commit cda0e4c

Please sign in to comment.