Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove param_refs and implicit_param_refs #4479

Open
wants to merge 11 commits into
base: trunk
Choose a base branch
from
Open

Remove param_refs and implicit_param_refs #4479

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f314f33
Remove param_refs and implicit_param_refs
geoffromer Nov 4, 2024
4a342e8
Respond to reviewer comments
geoffromer Nov 4, 2024
bf6bd19
Apply suggestions from reviewers
geoffromer Nov 4, 2024
15758ba
Respond to reviewer comments
geoffromer Nov 4, 2024
97427a8
Respond to reviewer comments
geoffromer Nov 12, 2024
7023ab4
Respond to reviewer comments.
geoffromer Nov 14, 2024
d2bda23
Apply suggestions from code review
geoffromer Nov 18, 2024
3b887fc
Respond to reviewer comments.
geoffromer Nov 18, 2024
9a68d88
Apply suggestions from code review
geoffromer Nov 20, 2024
e4b63c3
Apply suggestions from code review
geoffromer Nov 20, 2024
0821fb6
Respond to reviewer comments
geoffromer Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion toolchain/check/call.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ static auto ResolveCalleeInCall(Context& context, SemIR::LocId loc_id,
CalleeParamsInfo callee_info(entity);

// Check that the arity matches.
auto params = context.inst_blocks().GetOrEmpty(callee_info.param_refs_id);
auto params = context.inst_blocks().GetOrEmpty(callee_info.param_patterns_id);
if (arg_ids.size() != params.size()) {
CARBON_DIAGNOSTIC(CallArgCountMismatch, Error,
"{0} argument{0:s} passed to "
Expand Down
4 changes: 0 additions & 4 deletions toolchain/check/convert.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,18 +97,14 @@ auto ConvertForExplicitAs(Context& context, Parse::NodeId as_node,
struct CalleeParamsInfo {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to note, #4452 is now merged

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect the merge conflicts to be pretty straightforward, but I can do the merge during the review instead of the end if you like.

explicit CalleeParamsInfo(const SemIR::EntityWithParamsBase& callee)
: callee_loc(callee.latest_decl_id()),
implicit_param_refs_id(callee.implicit_param_refs_id),
implicit_param_patterns_id(callee.implicit_param_patterns_id),
param_refs_id(callee.param_refs_id),
param_patterns_id(callee.param_patterns_id) {}

// The location of the callee to use in diagnostics.
SemIRLoc callee_loc;
// The implicit parameters of the callee.
SemIR::InstBlockId implicit_param_refs_id;
SemIR::InstBlockId implicit_param_patterns_id;
// The explicit parameters of the callee.
SemIR::InstBlockId param_refs_id;
SemIR::InstBlockId param_patterns_id;
};

Expand Down
3 changes: 1 addition & 2 deletions toolchain/check/decl_name_stack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,9 @@ class DeclNameStack {
.first_param_node_id = name.first_param_node_id,
.last_param_node_id = name.last_param_node_id,
.pattern_block_id = name.pattern_block_id,
.implicit_param_refs_id = name.implicit_params_id,
.implicit_param_patterns_id = name.implicit_param_patterns_id,
.param_refs_id = name.params_id,
.param_patterns_id = name.param_patterns_id,
.calling_convention_params_id = name.calling_convention_params_id,
.is_extern = is_extern,
.extern_library_id = extern_library,
.non_owning_decl_id =
Expand Down
3 changes: 1 addition & 2 deletions toolchain/check/global_init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,9 @@ auto GlobalInit::Finalize() -> void {
.first_param_node_id = Parse::NodeId::Invalid,
.last_param_node_id = Parse::NodeId::Invalid,
.pattern_block_id = SemIR::InstBlockId::Empty,
.implicit_param_refs_id = SemIR::InstBlockId::Invalid,
.implicit_param_patterns_id = SemIR::InstBlockId::Invalid,
.param_refs_id = SemIR::InstBlockId::Empty,
.param_patterns_id = SemIR::InstBlockId::Empty,
.calling_convention_params_id = SemIR::InstBlockId::Empty,
.is_extern = false,
.extern_library_id = SemIR::LibraryNameId::Invalid,
.non_owning_decl_id = SemIR::InstId::Invalid,
Expand Down
34 changes: 17 additions & 17 deletions toolchain/check/handle_function.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,10 +182,8 @@ static auto BuildFunctionDecl(Context& context,
}

auto name = PopNameComponent(context, return_slot_pattern_id);
if (!name.params_id.is_valid()) {
CARBON_CHECK(!name.param_patterns_id.is_valid());
if (!name.param_patterns_id.is_valid()) {
context.TODO(node_id, "function with positional parameters");
name.params_id = SemIR::InstBlockId::Empty;
name.param_patterns_id = SemIR::InstBlockId::Empty;
}

Expand Down Expand Up @@ -342,26 +340,28 @@ static auto HandleFunctionDefinitionAfterSignature(
CheckFunctionReturnType(context, function.return_slot_id, function,
SemIR::SpecificId::Invalid);

auto params_to_complete =
context.inst_blocks().GetOrEmpty(function.calling_convention_params_id);
if (function.return_slot_id.is_valid()) {
// Exclude the return slot because it's diagnosed above.
params_to_complete = params_to_complete.drop_back();
}
// Check the parameter types are complete.
for (auto param_ref_id : llvm::concat<const SemIR::InstId>(
context.inst_blocks().GetOrEmpty(function.implicit_param_refs_id),
context.inst_blocks().GetOrEmpty(function.param_refs_id))) {
for (auto param_ref_id : params_to_complete) {
if (param_ref_id == SemIR::InstId::BuiltinError) {
continue;
}
auto param_info =
SemIR::Function::GetParamFromParamRefId(context.sem_ir(), param_ref_id);
jonmeow marked this conversation as resolved.
Show resolved Hide resolved

// The parameter types need to be complete.
context.TryToCompleteType(param_info.inst.type_id, [&] {
CARBON_DIAGNOSTIC(
IncompleteTypeInFunctionParam, Error,
"parameter has incomplete type {0} in function definition",
TypeOfInstId);
return context.emitter().Build(param_info.inst_id,
IncompleteTypeInFunctionParam,
param_info.inst_id);
});
context.TryToCompleteType(
context.insts().GetAs<SemIR::AnyParam>(param_ref_id).type_id, [&] {
CARBON_DIAGNOSTIC(
IncompleteTypeInFunctionParam, Error,
"parameter has incomplete type {0} in function definition",
TypeOfInstId);
return context.emitter().Build(
param_ref_id, IncompleteTypeInFunctionParam, param_ref_id);
});
}

context.node_stack().Push(node_id, function_id);
Expand Down
12 changes: 5 additions & 7 deletions toolchain/check/handle_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,14 +205,13 @@ static auto PopImplIntroducerAndParamsAsNameComponent(
auto [implicit_params_loc_id, implicit_param_patterns_id] =
context.node_stack().PopWithNodeIdIf<Parse::NodeKind::ImplForall>();

ParameterBlocks parameter_blocks{
.implicit_params_id = SemIR::InstBlockId::Invalid,
.params_id = SemIR::InstBlockId::Invalid,
.return_slot_id = SemIR::InstId::Invalid};
if (implicit_param_patterns_id) {
parameter_blocks =
auto parameter_blocks =
geoffromer marked this conversation as resolved.
Show resolved Hide resolved
CalleePatternMatch(context, *implicit_param_patterns_id,
SemIR::InstBlockId::Invalid, SemIR::InstId::Invalid);
CARBON_CHECK(parameter_blocks.calling_convention_params_id ==
SemIR::InstBlockId::Empty);
CARBON_CHECK(parameter_blocks.return_slot_id == SemIR::InstId::Invalid);
}

Parse::NodeId first_param_node_id =
Expand All @@ -225,12 +224,11 @@ static auto PopImplIntroducerAndParamsAsNameComponent(
.first_param_node_id = first_param_node_id,
.last_param_node_id = last_param_node_id,
.implicit_params_loc_id = implicit_params_loc_id,
.implicit_params_id = parameter_blocks.implicit_params_id,
.implicit_param_patterns_id =
implicit_param_patterns_id.value_or(SemIR::InstBlockId::Invalid),
.params_loc_id = Parse::NodeId::Invalid,
.params_id = SemIR::InstBlockId::Invalid,
.param_patterns_id = SemIR::InstBlockId::Invalid,
.calling_convention_params_id = SemIR::InstBlockId::Invalid,
.return_slot_pattern_id = SemIR::InstId::Invalid,
.return_slot_id = SemIR::InstId::Invalid,
.pattern_block_id = context.pattern_block_stack().Pop(),
Expand Down
132 changes: 5 additions & 127 deletions toolchain/check/import_ref.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,27 +667,6 @@ class ImportRefResolver {
return specific_id;
}

// Adds unresolved constants for each parameter's type to work_stack_.
auto LoadLocalParamConstantIds(SemIR::InstBlockId param_refs_id) -> void {
if (!param_refs_id.is_valid() ||
param_refs_id == SemIR::InstBlockId::Empty) {
return;
}

const auto& param_refs = import_ir_.inst_blocks().Get(param_refs_id);
for (auto inst_id : param_refs) {
GetLocalConstantId(import_ir_.insts().Get(inst_id).type_id());

// If the parameter is a symbolic binding, build the BindSymbolicName
// constant.
auto bind_id = inst_id;
auto bind_inst = import_ir_.insts().Get(bind_id);
if (bind_inst.Is<SemIR::BindSymbolicName>()) {
GetLocalConstantId(bind_id);
}
}
}

// Adds unresolved constants for each parameter's type to work_stack_.
auto LoadLocalPatternConstantIds(SemIR::InstBlockId param_patterns_id)
-> void {
Expand Down Expand Up @@ -718,10 +697,11 @@ class ImportRefResolver {
}
}

// Returns a version of param_refs_id localized to the current IR.
// Returns a version of param_patterns_id localized to the current IR.
//
// Must only be called after a call to GetLocalParamConstantIds(param_refs_id)
// has completed without adding any new work to work_stack_.
// Must only be called after a call to
// LoadLocalPatternConstantIds(param_patterns_id) has completed without adding
// any new work to work_stack_.
//
// TODO: This is inconsistent with the rest of this class, which expects
// the relevant constants to be explicitly passed in. That makes it
Expand All @@ -730,81 +710,6 @@ class ImportRefResolver {
// take a holistic look at how to balance those concerns. For example,
// could the same function be used to load the constants and use them, with
// a parameter to select between the two?
auto GetLocalParamRefsId(SemIR::InstBlockId param_refs_id)
-> SemIR::InstBlockId {
if (!param_refs_id.is_valid() ||
param_refs_id == SemIR::InstBlockId::Empty) {
return param_refs_id;
}
const auto& param_refs = import_ir_.inst_blocks().Get(param_refs_id);
llvm::SmallVector<SemIR::InstId> new_param_refs;
for (auto ref_id : param_refs) {
// Figure out the param structure. This echoes
// Function::GetParamFromParamRefId, and could use that function if we
// added `bool addr` and `InstId bind_inst_id` to its return `ParamInfo`.
// TODO: Consider a different parameter handling to simplify import logic.
auto inst = import_ir_.insts().Get(ref_id);

auto bind_id = ref_id;
auto param_id = ref_id;

auto bind_inst = inst.As<SemIR::AnyBindName>();
param_id = bind_inst.value_id;
inst = import_ir_.insts().Get(param_id);
auto param_inst = inst.As<SemIR::ValueParam>();

// Rebuild the param instruction.
auto entity_name =
import_ir_.entity_names().Get(bind_inst.entity_name_id);
auto name_id = GetLocalNameId(entity_name.name_id);
auto type_id = context_.GetTypeIdForTypeConstant(
GetLocalConstantIdChecked(param_inst.type_id));

auto new_param_id = context_.AddInstInNoBlock<SemIR::ValueParam>(
AddImportIRInst(param_id),
{.type_id = type_id,
.runtime_index = param_inst.runtime_index,
.pretty_name_id = GetLocalNameId(param_inst.pretty_name_id)});
switch (bind_inst.kind) {
case SemIR::BindName::Kind: {
auto entity_name_id = context_.entity_names().Add(
{.name_id = name_id,
.parent_scope_id = SemIR::NameScopeId::Invalid,
.bind_index = SemIR::CompileTimeBindIndex::Invalid});
new_param_id = context_.AddInstInNoBlock<SemIR::BindName>(
AddImportIRInst(bind_id), {.type_id = type_id,
.entity_name_id = entity_name_id,
.value_id = new_param_id});
break;
}
case SemIR::BindSymbolicName::Kind: {
// We already imported a constant value for this symbolic binding.
// We can reuse most of it, but update the value to point to our
// specific parameter, and preserve the constant value.
auto new_bind_inst = context_.insts().GetAs<SemIR::BindSymbolicName>(
context_.constant_values().GetInstId(
GetLocalConstantIdChecked(bind_id)));
new_bind_inst.value_id = new_param_id;
new_param_id = context_.AddInstInNoBlock(AddImportIRInst(bind_id),
new_bind_inst);
context_.constant_values().Set(new_param_id,
GetLocalConstantIdChecked(bind_id));
break;
}
default: {
CARBON_FATAL("Unexpected kind: {0}", bind_inst.kind);
}
}
new_param_refs.push_back(new_param_id);
}
return context_.inst_blocks().Add(new_param_refs);
}

// Returns a version of param_patterns_id localized to the current IR.
//
// Must only be called after a call to
// LoadLocalPatternConstantIds(param_patterns_id) has completed without adding
// any new work to work_stack_.
auto GetLocalParamPatternsId(SemIR::InstBlockId param_patterns_id)
-> SemIR::InstBlockId {
if (!param_patterns_id.is_valid() ||
Expand Down Expand Up @@ -1029,19 +934,14 @@ class ImportRefResolver {
.first_param_node_id = Parse::NodeId::Invalid,
.last_param_node_id = Parse::NodeId::Invalid,
.pattern_block_id = SemIR::InstBlockId::Invalid,
.implicit_param_refs_id = import_base.implicit_param_refs_id.is_valid()
? SemIR::InstBlockId::Empty
: SemIR::InstBlockId::Invalid,
.implicit_param_patterns_id =
import_base.implicit_param_patterns_id.is_valid()
? SemIR::InstBlockId::Empty
: SemIR::InstBlockId::Invalid,
.param_refs_id = import_base.param_refs_id.is_valid()
? SemIR::InstBlockId::Empty
: SemIR::InstBlockId::Invalid,
.param_patterns_id = import_base.param_patterns_id.is_valid()
? SemIR::InstBlockId::Empty
: SemIR::InstBlockId::Invalid,
.calling_convention_params_id = SemIR::InstBlockId::Invalid,
.is_extern = import_base.is_extern,
.extern_library_id = extern_library_id,
.non_owning_decl_id = import_base.non_owning_decl_id.is_valid()
Expand Down Expand Up @@ -1507,9 +1407,7 @@ class ImportRefResolver {

// Load constants for the definition.
auto parent_scope_id = GetLocalNameScopeId(import_class.parent_scope_id);
LoadLocalParamConstantIds(import_class.implicit_param_refs_id);
LoadLocalPatternConstantIds(import_class.implicit_param_patterns_id);
LoadLocalParamConstantIds(import_class.param_refs_id);
LoadLocalPatternConstantIds(import_class.param_patterns_id);
auto generic_data = GetLocalGenericData(import_class.generic_id);
auto self_const_id = GetLocalConstantId(import_class.self_type_id);
Expand All @@ -1527,11 +1425,8 @@ class ImportRefResolver {

auto& new_class = context_.classes().Get(class_id);
new_class.parent_scope_id = parent_scope_id;
new_class.implicit_param_refs_id =
GetLocalParamRefsId(import_class.implicit_param_refs_id);
new_class.implicit_param_patterns_id =
GetLocalParamPatternsId(import_class.implicit_param_patterns_id);
new_class.param_refs_id = GetLocalParamRefsId(import_class.param_refs_id);
new_class.param_patterns_id =
GetLocalParamPatternsId(import_class.param_patterns_id);
SetGenericData(import_class.generic_id, new_class.generic_id, generic_data);
Expand Down Expand Up @@ -1685,9 +1580,7 @@ class ImportRefResolver {
import_ir_.insts().Get(import_function.return_slot_id).type_id());
}
auto parent_scope_id = GetLocalNameScopeId(import_function.parent_scope_id);
LoadLocalParamConstantIds(import_function.implicit_param_refs_id);
LoadLocalPatternConstantIds(import_function.implicit_param_patterns_id);
LoadLocalParamConstantIds(import_function.param_refs_id);
LoadLocalPatternConstantIds(import_function.param_patterns_id);
auto generic_data = GetLocalGenericData(import_function.generic_id);

Expand All @@ -1698,12 +1591,8 @@ class ImportRefResolver {
// Add the function declaration.
auto& new_function = context_.functions().Get(function_id);
new_function.parent_scope_id = parent_scope_id;
new_function.implicit_param_refs_id =
GetLocalParamRefsId(import_function.implicit_param_refs_id);
new_function.implicit_param_patterns_id =
GetLocalParamPatternsId(import_function.implicit_param_patterns_id);
new_function.param_refs_id =
GetLocalParamRefsId(import_function.param_refs_id);
new_function.param_patterns_id =
GetLocalParamPatternsId(import_function.param_patterns_id);
new_function.return_slot_pattern_id =
Expand Down Expand Up @@ -1850,7 +1739,6 @@ class ImportRefResolver {

// Load constants for the definition.
auto parent_scope_id = GetLocalNameScopeId(import_impl.parent_scope_id);
LoadLocalParamConstantIds(import_impl.implicit_param_refs_id);
LoadLocalPatternConstantIds(import_impl.implicit_param_patterns_id);
auto generic_data = GetLocalGenericData(import_impl.generic_id);
auto self_const_id = GetLocalConstantId(
Expand All @@ -1864,12 +1752,8 @@ class ImportRefResolver {

auto& new_impl = context_.impls().Get(impl_id);
new_impl.parent_scope_id = parent_scope_id;
new_impl.implicit_param_refs_id =
GetLocalParamRefsId(import_impl.implicit_param_refs_id);
new_impl.implicit_param_patterns_id =
GetLocalParamPatternsId(import_impl.implicit_param_patterns_id);
CARBON_CHECK(!import_impl.param_refs_id.is_valid() &&
!new_impl.param_refs_id.is_valid());
SetGenericData(import_impl.generic_id, new_impl.generic_id, generic_data);

// Create instructions for self and constraint to hold the symbolic constant
Expand Down Expand Up @@ -2016,9 +1900,7 @@ class ImportRefResolver {

auto parent_scope_id =
GetLocalNameScopeId(import_interface.parent_scope_id);
LoadLocalParamConstantIds(import_interface.implicit_param_refs_id);
LoadLocalPatternConstantIds(import_interface.implicit_param_patterns_id);
LoadLocalParamConstantIds(import_interface.param_refs_id);
LoadLocalPatternConstantIds(import_interface.param_patterns_id);
auto generic_data = GetLocalGenericData(import_interface.generic_id);

Expand All @@ -2033,12 +1915,8 @@ class ImportRefResolver {

auto& new_interface = context_.interfaces().Get(interface_id);
new_interface.parent_scope_id = parent_scope_id;
new_interface.implicit_param_refs_id =
GetLocalParamRefsId(import_interface.implicit_param_refs_id);
new_interface.implicit_param_patterns_id =
GetLocalParamPatternsId(import_interface.implicit_param_patterns_id);
new_interface.param_refs_id =
GetLocalParamRefsId(import_interface.param_refs_id);
new_interface.param_patterns_id =
GetLocalParamPatternsId(import_interface.param_patterns_id);
SetGenericData(import_interface.generic_id, new_interface.generic_id,
Expand Down
Loading