Remove param_refs and implicit_param_refs #4479

geoffromer · 2024-11-04T16:45:02Z

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 regarding the decision to keep the return slot last in the SemIR calling convention, even though it goes first in the LLVM calling convention.

jonmeow

Should the PR description have some more explanation, like a link to the discussion about this and a mention of what it's being replaced with?

jonmeow · 2024-11-04T17:13:38Z

toolchain/check/convert.h

@@ -97,18 +97,14 @@ auto ConvertForExplicitAs(Context& context, Parse::NodeId as_node,
 struct CalleeParamsInfo {


Just to note, #4452 is now merged

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.

jonmeow · 2024-11-04T17:15:49Z

toolchain/sem_ir/entity_with_params_base.h

+  // A block containing, for each calling convention parameter (including the
+  // return slot), a reference to the instruction in the function's declaration
+  // block that represents that parameter. These instructions will be in the
+  // `AnyParam` category. This is not populated on imported entities, because it
+  // is relevant only for the function definition.
+  // TODO: can this be moved to `Function`, since it is not applicable to other
+  // kinds of entities?
+  InstBlockId calling_convention_params_id;


@zygoloid Can you just confirm that this is what you're expecting?

Just to note, one thing I'd particularly want to be sure of is that you both understand what "calling convention parameter" is, that it's a term I'm just not familiar with. To me, that sounds distinctly qualified from "implicit" or "explicit", not a union. If you're still considering names, I would suggest something like "full parameters" or "signature parameters" for this.

See pattern_match.h for the definition of that term (such as it is). Do you think it would make sense to move that here?

I think this would be the more central location to document a a term. However, I've also brought this up on #naming.

toolchain/sem_ir/entity_with_params_base.h

toolchain/lower/file_context.cpp

toolchain/check/pattern_match.h

toolchain/check/pattern_match.cpp

toolchain/check/handle_impl.cpp

toolchain/check/handle_function.cpp

geoffromer

Should the PR description have some more explanation, like a link to the discussion about this and a mention of what it's being replaced with?

Good point, I've fleshed out the description, but what discussion are you referring to?

geoffromer · 2024-11-04T18:48:21Z

toolchain/check/convert.h

@@ -97,18 +97,14 @@ auto ConvertForExplicitAs(Context& context, Parse::NodeId as_node,
 struct CalleeParamsInfo {


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.

toolchain/check/handle_function.cpp

toolchain/check/handle_impl.cpp

toolchain/check/pattern_match.cpp

toolchain/check/pattern_match.h

toolchain/lower/file_context.cpp

toolchain/check/pattern_match.cpp

toolchain/lower/file_context.cpp

toolchain/check/pattern_match.cpp

jonmeow · 2024-11-04T21:41:52Z

Should the PR description have some more explanation, like a link to the discussion about this and a mention of what it's being replaced with?

Good point, I've fleshed out the description, but what discussion are you referring to?

https://discord.com/channels/655572317891461132/655578254970716160/1300545448909738125

jonmeow · 2024-11-04T21:46:59Z

toolchain/sem_ir/entity_with_params_base.h

+  // parameter. These parameters appear in declaration order: `self` (if
+  // present), then the explicit runtime parameters, then the return slot (which


Right now, the only implicit parameter the toolchain supports is self. However, once more are supported, won't those become part of the calling convention? Also, are you intending to exclude non-runtime explicit parameters (do we have those?)?

You might consider something like:

Suggested change

// parameter. These parameters appear in declaration order: `self` (if

// present), then the explicit runtime parameters, then the return slot (which

// parameter. Parameters are in declaration order: implicit parameters, explicit parameters, then the return slot (which

I'm intending to exclude all non-runtime parameters, because they aren't part of the calling convention (i.e. they aren't passed as part of the function call). Unless I'm missing something, self is the only non-compile-time parameter that can appear in the implicit parameter list.

jonmeow · 2024-11-04T21:55:29Z

toolchain/check/name_component.h

  SemIR::InstBlockId param_patterns_id;

+  // The calling convention parameters of the function. These will all be


Suggested change

// The calling convention parameters of the function. These will all be

// The calling convention parameters of the entity. These will all be

Won't this be set for all parameterized entities, not only functions?

I believe functions are the only entities that have a calling convention (because they're the only entities that can be called with runtime arguments).

Would it be reasonable to add a TODO similar to the one for the return slot fields here?

I think it'd also be useful to specify more explicitly what you mean by "calling convention parameters" here. Maybe overall something like:

The calling convention parameters of this entity, if it is a function. These are the parameters that correspond to the arguments in a Call instruction that calls this function. These will all be instances of AnyParam.

I also wonder if a shorter name like call_params_id or maybe call_inst_params_id might capture the same meaning. ("Calling convention" sounds to me like it might be talking about ABI, which I think is separate.)

Hm, or maybe this should reference EntityWithParamsBase for the full explanation of this field. I think the TODO and the change from "the function" to "the entity if it is a function" would still be useful here, though.

Done, and renamed to call_params_id as discussed at today's meeting.

jonmeow · 2024-11-04T22:01:18Z

toolchain/sem_ir/entity_with_params_base.h

+  // A block containing, for each calling convention parameter (including the
+  // return slot), a reference to the instruction in the function's declaration
+  // block that represents that parameter. These instructions will be in the
+  // `AnyParam` category. This is not populated on imported entities, because it
+  // is relevant only for the function definition.
+  // TODO: can this be moved to `Function`, since it is not applicable to other
+  // kinds of entities?
+  InstBlockId calling_convention_params_id;


Just to note, one thing I'd particularly want to be sure of is that you both understand what "calling convention parameter" is, that it's a term I'm just not familiar with. To me, that sounds distinctly qualified from "implicit" or "explicit", not a union. If you're still considering names, I would suggest something like "full parameters" or "signature parameters" for this.

jonmeow

Just to note, I'm basically fine with this, but would like to make sure zygoloid is happy with this, particularly the naming (which throws me).

Co-authored-by: Jon Ross-Perkins <[email protected]>

zygoloid · 2024-11-11T21:50:14Z

toolchain/sem_ir/entity_with_params_base.h

  // instruction in the entity's pattern block that depends on all other
  // pattern insts pertaining to that parameter.
  InstBlockId param_patterns_id;
+  // A block containing, for each calling convention parameter, a reference to


I think it'd be useful to explain what you mean by "calling convention parameter" here: that is, this list is the parameters corresponding to the arguments in a Call instruction.

jonmeow

At a high-level, I think we were particularly discussing "Call parameters", i.e. referring to the specific entity, rather than "call parameters". I think that makes a difference in documentation, with one example below. What do you think of changing how you're referring to that?

jonmeow · 2024-11-13T22:33:58Z

toolchain/sem_ir/entity_with_params_base.h

+  // `AnyParam` insts that represent the function's call parameters. The "call
+  // parameters" are the parameters corresponding to the arguments that are
+  // passed to a `Call` inst, so they include `self` (if applicable) and the


Why does this say "call parameters" instead of "Call"? Had you considered something like:

Suggested change

// `AnyParam` insts that represent the function's call parameters. The "call

// parameters" are the parameters corresponding to the arguments that are

// passed to a `Call` inst, so they include `self` (if applicable) and the

// `AnyParam` insts that represent the function's `Call` parameters. They include `self` (if applicable) and the

I want to be a little more explicit about the fact that I'm defining this as a term of art, particularly so that it can be reused in places like name_component.h. It hadn't occurred to me to say "Call" instead of "call", but I suppose that works; done.

I think extra documentation can help, but I want to focus on this:

this block consists of references to the AnyParam insts that represent the function's Call parameters. The "Call parameters" are the parameters corresponding to the arguments that are passed to a Call inst

Isn't this just saying that the Call parameters are parameters to the Call instruction? I end up a little uncertain because, to me, when I see something that looks repetitive, I tend to assume that I'm actually misreading and missing a nuance. As a consequence, the repetition actually makes this harder for me to read, not easier. Maybe there's some more terse way of stating this, such as "Call inst parameters" if you're concerned about non-instruction interpretations of just "Call"?

Although, this also raises another question. Maybe this comment should be on Call so that when we say "Call parameters" the answer is found on Call without needing to say "look at EntityWithParamsBase"? Sorry about this, since I think this has already moved around a little.

Isn't this just saying that the Call parameters are parameters to the Call instruction?

I wouldn't put it that way, no. EntityWithParamsBase represents part of the declaration of a callable entity; it doesn't represent a particular call to that entity, so there's no Call inst in "scope" here. By the same token, Call doesn't have parameters, it has arguments. Does that make sense?

Although, this also raises another question. Maybe this comment should be on Call so that when we say "Call parameters" the answer is found on Call without needing to say "look at EntityWithParamsBase"? Sorry about this, since I think this has already moved around a little.

That could work, but I think it would be awkward, for the same reason: the Call inst represents a particular call to a callable entity, so I think it would feel like a non sequitur to talk about how that callable entity represents its parameters in its own IR.

(While looking at this I noticed that the Call documentation is out of date, so I fixed that.)

Isn't this just saying that the Call parameters are parameters to the Call instruction?

I wouldn't put it that way, no. EntityWithParamsBase represents part of the declaration of a callable entity; it doesn't represent a particular call to that entity, so there's no Call inst in "scope" here. By the same token, Call doesn't have parameters, it has arguments. Does that make sense?

FWIW, here's a different phrasing that I don't know if it'd work for you:

`Call` parameters correspond to `Call::args_id`, but in parameter form.

I'll leave it to you whether to adopt something like that.

Although, this also raises another question. Maybe this comment should be on Call so that when we say "Call parameters" the answer is found on Call without needing to say "look at EntityWithParamsBase"? Sorry about this, since I think this has already moved around a little.

That could work, but I think it would be awkward, for the same reason: the Call inst represents a particular call to a callable entity, so I think it would feel like a non sequitur to talk about how that callable entity represents its parameters in its own IR.

(While looking at this I noticed that the Call documentation is out of date, so I fixed that.)

To be clear here, what I really mean is instead of having things like "(see entity_with_params_base.h for the definition of that term)." in pattern_match.h, maybe having it on Call would mean that people seeing "Call parameters" and wondering what that means would be able to find it on Call without the explicit reference. I'm not meaning to argue the detail in this half of the comment.

FWIW, here's a different phrasing that I don't know if it'd work for you:

`Call` parameters correspond to `Call::args_id`, but in parameter form.

I'll leave it to you whether to adopt something like that.

I'm afraid the "in parameter form" part doesn't work for me -- they don't just have different forms, they have different content, because they represent categorically different entities.

That said, I don't want to just shoot down your suggestions; this concept is kind of subtle, so it's important for the explanation to be clear, and evidently it isn't. If you have any thoughts about how to fix it, I'm happy to iterate more in a follow-up PR.

jonmeow · 2024-11-13T22:34:56Z

toolchain/sem_ir/entity_with_params_base.h

+  // passed to a `Call` inst, so they include `self` (if applicable) and the
+  // return slot, but do not include compile-time parameters.
+  //
+  // The parameters appear in declaration order: `self` (if present), then the
+  // explicit runtime parameters, then the return slot (which is "declared" by
+  // the function's return type declaration). This is not populated on imported


You note the things which are included twice here, had you considered removing one of the mentions?

I thought about it, but I couldn't find a way to do it without creating other problems. When I'm defining "Call parameters" it seems important to give examples of how it differs from other meanings of "parameters", and at some point I need to specify the parameter order fairly precisely since all the code that uses this field needs to agree on that, but combining the two forces me to front-load the ordering specification, which seems like it's dumping too much information on the reader all at once.

The self example in the first paragraph wasn't adding much, so I've at least removed that, but I'm not sure what else to do.

toolchain/lower/file_context.cpp

toolchain/sem_ir/ids.h

toolchain/check/pattern_match.h

jonmeow · 2024-11-14T19:31:22Z

toolchain/check/handle_impl.cpp

+    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);


Can you elaborate what's going on here? Previously, the result of parameter_blocks.implicit_params_id was retained. What's the effect of running it when the result is discarded?

The primary job of CalleePatternMatch is to emit the pattern-match IR, and we still want to do that here, even though we no longer use the return value.

Thanks, what do you think of adding a comment here to explain the code? I've taken a stab above to try to clarify what I'm requesting.

Done, with somewhat different wording.

toolchain/sem_ir/entity_with_params_base.h

jonmeow · 2024-11-14T19:42:14Z

toolchain/sem_ir/entity_with_params_base.h

+  // `AnyParam` insts that represent the function's call parameters. The "call
+  // parameters" are the parameters corresponding to the arguments that are
+  // passed to a `Call` inst, so they include `self` (if applicable) and the


I think extra documentation can help, but I want to focus on this:

this block consists of references to the AnyParam insts that represent the function's Call parameters. The "Call parameters" are the parameters corresponding to the arguments that are passed to a Call inst

Isn't this just saying that the Call parameters are parameters to the Call instruction? I end up a little uncertain because, to me, when I see something that looks repetitive, I tend to assume that I'm actually misreading and missing a nuance. As a consequence, the repetition actually makes this harder for me to read, not easier. Maybe there's some more terse way of stating this, such as "Call inst parameters" if you're concerned about non-instruction interpretations of just "Call"?

Although, this also raises another question. Maybe this comment should be on Call so that when we say "Call parameters" the answer is found on Call without needing to say "look at EntityWithParamsBase"? Sorry about this, since I think this has already moved around a little.

This is at a point where it's getting difficult to read, and #4479 is adding a large block comment. In order to more clearly delineate members, add blank lines between.

Co-authored-by: Jon Ross-Perkins <[email protected]>

jonmeow

LGTM. Some lingering comments, including one more "call parameters" I noticed, but generally I think this is fine to merge. To be clear, feel free to merge regardless of how you resolve remaining comments. Sorry about the long review here.

jonmeow · 2024-11-19T23:07:25Z

toolchain/check/handle_impl.cpp

+    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);


Thanks, what do you think of adding a comment here to explain the code? I've taken a stab above to try to clarify what I'm requesting.

toolchain/check/handle_impl.cpp

toolchain/check/pattern_match.h

jonmeow · 2024-11-19T23:26:00Z

toolchain/sem_ir/entity_with_params_base.h

+  // `AnyParam` insts that represent the function's call parameters. The "call
+  // parameters" are the parameters corresponding to the arguments that are
+  // passed to a `Call` inst, so they include `self` (if applicable) and the


Isn't this just saying that the Call parameters are parameters to the Call instruction?

I wouldn't put it that way, no. EntityWithParamsBase represents part of the declaration of a callable entity; it doesn't represent a particular call to that entity, so there's no Call inst in "scope" here. By the same token, Call doesn't have parameters, it has arguments. Does that make sense?

FWIW, here's a different phrasing that I don't know if it'd work for you:

`Call` parameters correspond to `Call::args_id`, but in parameter form.

I'll leave it to you whether to adopt something like that.

Although, this also raises another question. Maybe this comment should be on Call so that when we say "Call parameters" the answer is found on Call without needing to say "look at EntityWithParamsBase"? Sorry about this, since I think this has already moved around a little.

That could work, but I think it would be awkward, for the same reason: the Call inst represents a particular call to a callable entity, so I think it would feel like a non sequitur to talk about how that callable entity represents its parameters in its own IR.

(While looking at this I noticed that the Call documentation is out of date, so I fixed that.)

To be clear here, what I really mean is instead of having things like "(see entity_with_params_base.h for the definition of that term)." in pattern_match.h, maybe having it on Call would mean that people seeing "Call parameters" and wondering what that means would be able to find it on Call without the explicit reference. I'm not meaning to argue the detail in this half of the comment.

toolchain/sem_ir/typed_insts.h

Co-authored-by: Jon Ross-Perkins <[email protected]>

Remove param_refs and implicit_param_refs

f314f33

github-actions bot requested a review from jonmeow November 4, 2024 16:45

github-actions bot added the toolchain label Nov 4, 2024

jonmeow reviewed Nov 4, 2024

View reviewed changes

Respond to reviewer comments

4a342e8

geoffromer commented Nov 4, 2024

View reviewed changes

jonmeow reviewed Nov 4, 2024

View reviewed changes

toolchain/check/pattern_match.cpp Show resolved Hide resolved

toolchain/lower/file_context.cpp Outdated Show resolved Hide resolved

toolchain/check/pattern_match.cpp Outdated Show resolved Hide resolved

jonmeow reviewed Nov 4, 2024

View reviewed changes

geoffromer and others added 2 commits November 4, 2024 15:48

Apply suggestions from reviewers

bf6bd19

Co-authored-by: Jon Ross-Perkins <[email protected]>

Respond to reviewer comments

15758ba

geoffromer requested a review from zygoloid November 4, 2024 23:52

zygoloid reviewed Nov 11, 2024

View reviewed changes

Respond to reviewer comments

97427a8

geoffromer requested review from jonmeow and zygoloid November 12, 2024 23:42

jonmeow reviewed Nov 13, 2024

View reviewed changes

Respond to reviewer comments.

7023ab4

jonmeow reviewed Nov 14, 2024

View reviewed changes

jonmeow mentioned this pull request Nov 15, 2024

Add blank lines between EntityWithParamsBase members. #4536

Merged

geoffromer and others added 2 commits November 18, 2024 10:12

Apply suggestions from code review

d2bda23

Co-authored-by: Jon Ross-Perkins <[email protected]>

Respond to reviewer comments.

3b887fc

geoffromer requested a review from jonmeow November 19, 2024 21:12

jonmeow approved these changes Nov 19, 2024

View reviewed changes

jonmeow reviewed Nov 20, 2024

View reviewed changes

toolchain/sem_ir/typed_insts.h Outdated Show resolved Hide resolved

geoffromer and others added 3 commits November 19, 2024 16:14

Apply suggestions from code review

9a68d88

Co-authored-by: Jon Ross-Perkins <[email protected]>

Apply suggestions from code review

e4b63c3

Co-authored-by: Jon Ross-Perkins <[email protected]>

Respond to reviewer comments

0821fb6

		@@ -97,18 +97,14 @@ auto ConvertForExplicitAs(Context& context, Parse::NodeId as_node,
		struct CalleeParamsInfo {

		// parameter. These parameters appear in declaration order: `self` (if
		// present), then the explicit runtime parameters, then the return slot (which

	// parameter. These parameters appear in declaration order: `self` (if
	// present), then the explicit runtime parameters, then the return slot (which
	// parameter. Parameters are in declaration order: implicit parameters, explicit parameters, then the return slot (which

		SemIR::InstBlockId param_patterns_id;

		// The calling convention parameters of the function. These will all be

Remove param_refs and implicit_param_refs #4479

Are you sure you want to change the base?

Remove param_refs and implicit_param_refs #4479

Conversation

geoffromer commented Nov 4, 2024 • edited Loading

jonmeow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffromer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow commented Nov 4, 2024

jonmeow Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygoloid Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

jonmeow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

jonmeow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffromer commented Nov 4, 2024 •

edited

Loading

jonmeow Nov 4, 2024 •

edited

Loading

jonmeow Nov 4, 2024 •

edited

Loading

zygoloid Nov 11, 2024 •

edited

Loading

jonmeow Nov 4, 2024 •

edited

Loading

jonmeow Nov 14, 2024 •

edited

Loading

jonmeow Nov 14, 2024 •

edited

Loading