Skip to content

Commit

Permalink
Remove param_refs and implicit_param_refs (#4479)
Browse files Browse the repository at this point in the history 
This introduces `calling_convention_param_ids`, a single block that
consolidates all the information that was being used by consumers of
`param_refs` and `implicit_param_refs`, in a form that's easier to
produce and typically easier to consume.

See also [this Discord
discussion](https://discord.com/channels/655572317891461132/655578254970716160/1300545448909738125)
regarding the decision to keep the return slot last in the SemIR calling
convention, even though it goes first in the LLVM calling convention.

---------

Co-authored-by: Jon Ross-Perkins <[email protected]>
  • Loading branch information
@geoffromer @jonmeow
geoffromer and jonmeow authored Nov 22, 2024
1 parent 17272cf commit 4f816dd
Show file tree
Hide file tree
Showing 38 changed files with 940 additions and 1,105 deletions.
2 changes: 1 addition & 1 deletion toolchain/check/call.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ static auto ResolveCalleeInCall(Context& context, SemIR::LocId loc_id,
llvm::ArrayRef<SemIR::InstId> arg_ids)
-> std::optional<SemIR::SpecificId> {
// Check that the arity matches.
auto params = context.inst_blocks().GetOrEmpty(entity.param_refs_id);
auto params = context.inst_blocks().GetOrEmpty(entity.param_patterns_id);
if (arg_ids.size() != params.size()) {
CARBON_DIAGNOSTIC(CallArgCountMismatch, Error,
"{0} argument{0:s} passed to "
Expand Down
3 changes: 1 addition & 2 deletions toolchain/check/decl_name_stack.h
View file
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,
.call_params_id = name.call_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
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,
.call_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
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 @@ -341,26 +339,28 @@ static auto HandleFunctionDefinitionAfterSignature(
CheckFunctionReturnType(context, function.return_slot_id, function,
SemIR::SpecificId::Invalid);

auto params_to_complete =
context.inst_blocks().GetOrEmpty(function.call_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::BuiltinErrorInst) {
continue;
}
auto param_info =
SemIR::Function::GetParamFromParamRefId(context.sem_ir(), param_ref_id);

// 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
13 changes: 6 additions & 7 deletions toolchain/check/handle_impl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,14 +198,14 @@ 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 =
// Emit the `forall` match. This shouldn't produce any `Call` params,
// because `impl`s are never actually called at runtime.
auto parameter_blocks =
CalleePatternMatch(context, *implicit_param_patterns_id,
SemIR::InstBlockId::Invalid, SemIR::InstId::Invalid);
CARBON_CHECK(parameter_blocks.call_params_id == SemIR::InstBlockId::Empty);
CARBON_CHECK(parameter_blocks.return_slot_id == SemIR::InstId::Invalid);
}

Parse::NodeId first_param_node_id =
Expand All @@ -218,12 +218,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,
.call_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
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 @@ -1035,19 +940,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,
.call_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 @@ -1527,9 +1427,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 @@ -1547,11 +1445,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 @@ -1705,9 +1600,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 @@ -1718,12 +1611,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 @@ -1874,7 +1763,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 @@ -1888,12 +1776,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 @@ -2043,9 +1927,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 @@ -2060,12 +1942,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

0 comments on commit 4f816dd

Please sign in to comment.