Consolidate caller match in one function call #4446
Conversation
There was a problem hiding this comment.
The logic looks good, just a few things I'm noticing here.
One comment I can't make on an appropriate line, although it comes up due to EmitPatternMatch edits:
In pattern_match.cpp, you have a broad anonymous namespace. Should that be refined to only contain the types, per internal linkage guidance? Note that GetPrettyName in particular should probably be static, but is not. I noticed this due to EmitPatternMatch, which perhaps should also be moved out of the anonymous namespace.
| // A reference to the instruction in the entity's pattern block that | ||
| // depends on all other pattern insts pertaining to the return slot pattern. | ||
| // TODO: should this go on Function, like return_slot_id? | ||
| InstId return_slot_pattern_id; |
There was a problem hiding this comment.
My expectation would be that yes, this should kept as function-specific. I assume this is for ConvertCallArgs, why aren't you adding it as a parameter there (similar to return_slot_arg_id)?
There was a problem hiding this comment.
On a practical level: because it needs to be propagated into EntityParamsInfo, and at the point where EntityParamsInfo is created, we don't necessarily have a Function, we just have an EntityWithParamsBase. On a more conceptual level: because I'm trying to model the return slot pattern as a parameter, so it belongs with the other parameters. From that point of view, the problem is that return_slot_id is in the wrong place: it belongs with param_refs_id.
I'm still in the process of working out where all of these variables should go; we're definitely passing around more than we need (e.g. the non-pattern members of EntityParamsInfo are basically unused), and even the stuff we need isn't necessarily in the form that's most helpful (e.g. I suspect that param_refs_id should point to the Param insts, not the BindNames). I intend to resolve this TODO as part of that work.
There was a problem hiding this comment.
Can you elaborate on the practical issue? I think we really should avoid putting Function-specific fields on EntityWithParams, and as far as I can tell it's easy to pipe it the same as return_slot_id. I understand you may want to merge it with params, but I see no friction with keeping the Function-specific logic in one place, and I do think the divergence between return_slot_id and return_slot_pattern_id handling isn't necessary, even as a TODO.
Where you say there's a practical issue because there's not necessarily a Function, can you please be specific about where? While examining this, I couldn't find where you'd be running into that issue.
Here's the specific diff, which I see as passing tests:
diff --git a/toolchain/check/call.cpp b/toolchain/check/call.cpp
index 645ed1ce8..cfd31ac65 100644
--- a/toolchain/check/call.cpp
+++ b/toolchain/check/call.cpp
@@ -210,9 +210,10 @@ auto PerformCall(Context& context, SemIR::LocId loc_id, SemIR::InstId callee_id,
}
// Convert the arguments to match the parameters.
- auto converted_args_id = ConvertCallArgs(
- context, loc_id, callee_function.self_id, arg_ids, return_slot_arg_id,
- CalleeParamsInfo(callable), *callee_specific_id);
+ auto converted_args_id =
+ ConvertCallArgs(context, loc_id, callee_function.self_id, arg_ids,
+ return_slot_arg_id, callable.return_slot_pattern_id,
+ CalleeParamsInfo(callable), *callee_specific_id);
auto call_inst_id =
context.AddInst<SemIR::Call>(loc_id, {.type_id = return_info.type_id,
.callee_id = callee_id,
diff --git a/toolchain/check/convert.cpp b/toolchain/check/convert.cpp
index 29831532b..813e8d22c 100644
--- a/toolchain/check/convert.cpp
+++ b/toolchain/check/convert.cpp
@@ -1141,6 +1141,7 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id,
SemIR::InstId self_id,
llvm::ArrayRef<SemIR::InstId> arg_refs,
SemIR::InstId return_slot_arg_id,
+ SemIR::InstId return_slot_pattern_id,
const CalleeParamsInfo& callee,
SemIR::SpecificId callee_specific_id)
-> SemIR::InstBlockId {
@@ -1174,9 +1175,9 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id,
self_id = SemIR::InstId::BuiltinError;
}
- return CallerPatternMatch(
- context, callee_specific_id, self_param_id, callee.param_patterns_id,
- callee.return_slot_pattern_id, self_id, arg_refs, return_slot_arg_id);
+ return CallerPatternMatch(context, callee_specific_id, self_param_id,
+ callee.param_patterns_id, return_slot_pattern_id,
+ self_id, arg_refs, return_slot_arg_id);
}
auto ExprAsType(Context& context, SemIR::LocId loc_id, SemIR::InstId value_id)
diff --git a/toolchain/check/convert.h b/toolchain/check/convert.h
index 8254f460f..77ac3de3f 100644
--- a/toolchain/check/convert.h
+++ b/toolchain/check/convert.h
@@ -99,8 +99,7 @@ struct CalleeParamsInfo {
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),
- return_slot_pattern_id(callee.return_slot_pattern_id) {}
+ param_patterns_id(callee.param_patterns_id) {}
// The location of the callee to use in diagnostics.
SemIRLoc callee_loc;
@@ -110,8 +109,6 @@ struct CalleeParamsInfo {
// The explicit parameters of the callee.
SemIR::InstBlockId param_refs_id;
SemIR::InstBlockId param_patterns_id;
- // The return slot pattern of the callee.
- SemIR::InstId return_slot_pattern_id;
};
// Implicitly converts a set of arguments to match the parameter types in a
@@ -121,6 +118,7 @@ auto ConvertCallArgs(Context& context, SemIR::LocId call_loc_id,
SemIR::InstId self_id,
llvm::ArrayRef<SemIR::InstId> arg_refs,
SemIR::InstId return_slot_arg_id,
+ SemIR::InstId return_slot_pattern_id,
const CalleeParamsInfo& callee,
SemIR::SpecificId callee_specific_id)
-> SemIR::InstBlockId;
diff --git a/toolchain/check/decl_name_stack.h b/toolchain/check/decl_name_stack.h
index 43917d22f..239e6cc3b 100644
--- a/toolchain/check/decl_name_stack.h
+++ b/toolchain/check/decl_name_stack.h
@@ -105,7 +105,6 @@ class DeclNameStack {
.implicit_param_patterns_id = name.implicit_param_patterns_id,
.param_refs_id = name.params_id,
.param_patterns_id = name.param_patterns_id,
- .return_slot_pattern_id = name.return_slot_pattern_id,
.is_extern = is_extern,
.extern_library_id = extern_library,
.non_owning_decl_id =
diff --git a/toolchain/check/global_init.cpp b/toolchain/check/global_init.cpp
index 86556484e..5fba84fba 100644
--- a/toolchain/check/global_init.cpp
+++ b/toolchain/check/global_init.cpp
@@ -45,7 +45,6 @@ auto GlobalInit::Finalize() -> void {
.implicit_param_patterns_id = SemIR::InstBlockId::Invalid,
.param_refs_id = SemIR::InstBlockId::Empty,
.param_patterns_id = SemIR::InstBlockId::Empty,
- .return_slot_pattern_id = SemIR::InstId::Invalid,
.is_extern = false,
.extern_library_id = SemIR::LibraryNameId::Invalid,
.non_owning_decl_id = SemIR::InstId::Invalid,
diff --git a/toolchain/check/handle_function.cpp b/toolchain/check/handle_functi
on.cpp
index 1f88f5bcc..edde3033c 100644
--- a/toolchain/check/handle_function.cpp
+++ b/toolchain/check/handle_function.cpp
@@ -102,6 +102,7 @@ static auto MergeFunctionRedecl(Context& context, SemIRLoc n
ew_loc,
// match IDs in the signature.
prev_function.MergeDefinition(new_function);
prev_function.return_slot_id = new_function.return_slot_id;
+ prev_function.return_slot_pattern_id = new_function.return_slot_pattern_id;
}
if ((prev_import_ir_id.is_valid() && !new_is_import)) {
ReplacePrevInstForMerge(context, new_function.parent_scope_id,
@@ -237,6 +238,7 @@ static auto BuildFunctionDecl(Context& context,
SemIR::Function{{name_context.MakeEntityWithParamsBase(
name, decl_id, is_extern, introducer.extern_library)}
,
{.return_slot_id = name.return_slot_id,
+ .return_slot_pattern_id = name.return_slot_pattern_id,
.virtual_modifier = virtual_modifier}};
if (is_definition) {
function_info.definition_id = decl_id;
diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp
index 686eb6d13..b3a89afd2 100644
--- a/toolchain/check/import_ref.cpp
+++ b/toolchain/check/import_ref.cpp
@@ -1042,7 +1042,6 @@ class ImportRefResolver {
.param_patterns_id = import_base.param_patterns_id.is_valid()
? SemIR::InstBlockId::Empty
: SemIR::InstBlockId::Invalid,
- .return_slot_pattern_id = SemIR::InstId::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()
diff --git a/toolchain/sem_ir/entity_with_params_base.h b/toolchain/sem_ir/entit
y_with_params_base.h
index 784de3a9c..abbe83944 100644
--- a/toolchain/sem_ir/entity_with_params_base.h
+++ b/toolchain/sem_ir/entity_with_params_base.h
@@ -49,7 +49,6 @@ struct EntityWithParamsBase {
implicit_param_patterns_id = definition.implicit_param_patterns_id;
param_refs_id = definition.param_refs_id;
param_patterns_id = definition.param_patterns_id;
- return_slot_pattern_id = definition.return_slot_pattern_id;
definition_id = definition.definition_id;
}
@@ -111,10 +110,6 @@ struct EntityWithParamsBase {
// instruction in the entity's pattern block that depends on all other
// pattern insts pertaining to that parameter.
InstBlockId param_patterns_id;
- // A reference to the instruction in the entity's pattern block that
- // depends on all other pattern insts pertaining to the return slot pattern.
- // TODO: should this go on Function, like return_slot_id?
- InstId return_slot_pattern_id;
// True if declarations are `extern`.
bool is_extern;
// For an `extern library` declaration, the library name.
diff --git a/toolchain/sem_ir/function.h b/toolchain/sem_ir/function.h
index 299f73a6d..6d56d1401 100644
--- a/toolchain/sem_ir/function.h
+++ b/toolchain/sem_ir/function.h
@@ -26,6 +26,10 @@ struct FunctionFields {
// always present if the function has a declared return type.
InstId return_slot_id;
+ // A reference to the instruction in the entity's pattern block that
+ // depends on all other pattern insts pertaining to the return slot pattern.
+ InstId return_slot_pattern_id = InstId::Invalid;
+
// Which, if any, virtual modifier (virtual, abstract, or impl) is applied to
// this function.
VirtualModifier virtual_modifier;
There was a problem hiding this comment.
I see, I hadn't considered removing it from CalleeParamsInfo (which is what I meant when I said EntityParamsInfo previously). That seems awkward, because this really belongs with the other parameters, but I suppose it's more limited in scope than putting it in EntityWithParams. Done.
There was a problem hiding this comment.
Thanks, and I've sent off #4452 to remove CalleeParamsInfo entirely. I think generics have been refactored in a way that removes its value-add.
Co-authored-by: Jon Ross-Perkins <[email protected]>
There was a problem hiding this comment.
In pattern_match.cpp, you have a broad anonymous namespace. Should that be refined to only contain the types, per internal linkage guidance? Note that
GetPrettyNamein particular should probably bestatic, but is not. I noticed this due toEmitPatternMatch, which perhaps should also be moved out of the anonymous namespace.
I've made GetPrettyName static, but I think EmitPatternMatch has to stay in the anonymous namespace because it's a member of a class in the anonymous namespace.
| // A reference to the instruction in the entity's pattern block that | ||
| // depends on all other pattern insts pertaining to the return slot pattern. | ||
| // TODO: should this go on Function, like return_slot_id? | ||
| InstId return_slot_pattern_id; |
There was a problem hiding this comment.
On a practical level: because it needs to be propagated into EntityParamsInfo, and at the point where EntityParamsInfo is created, we don't necessarily have a Function, we just have an EntityWithParamsBase. On a more conceptual level: because I'm trying to model the return slot pattern as a parameter, so it belongs with the other parameters. From that point of view, the problem is that return_slot_id is in the wrong place: it belongs with param_refs_id.
I'm still in the process of working out where all of these variables should go; we're definitely passing around more than we need (e.g. the non-pattern members of EntityParamsInfo are basically unused), and even the stuff we need isn't necessarily in the form that's most helpful (e.g. I suspect that param_refs_id should point to the Param insts, not the BindNames). I intend to resolve this TODO as part of that work.
Have you tried moving it out? My expectation would be that the declaration of the containing class determines the linkage, not the definition of a member function. I think it'd be helpful to ensure the anonymous namespace has the minimal size required, so that the contents are clearer and more are less likely to be added by accident. Note you can observe this compiling for example at: https://cpp.compiler-explorer.com/z/n3G1E3boG |
Scanning, we also do this elsewhere, like |
It's surprising and counterintuitive to me that this works, which doesn't seem like a good sign for readability, but I will defer to existing practice. |
Nevermind, now I see the full text, it doesn't look like a flake -- please update the tests as needed (and then feel free to merge). |
I'm seeing three issues with CalleeParamsInfo: 1. Although ResolveCalleeInCall had been extracted out and the EntityWithParamsBase could be used directly, that had not been cleaned up. 2. implicit_param_refs_id is unused; param_refs_id was only used by ResolveCalleeInCall. The factoring as a struct seems to be obscuring what's used and what isn't (creating unnecessary copies). 3. On #4446, CalleeParamsInfo seemed to be obfuscating what ConvertCallArgs actually worked with (a Function, not a generic entity). I'm thinking that just removing CalleeParamsInfo is the best resolution here, it looks like it's tripping people up more than it's helping. Note, I think Function used to be named Callable, which was where the "callable" name originally came from (I might be wrong about this). But "function" seems clearer about the type now.
No description provided.