Initial support for binding patterns in SemIR #4221

geoffromer · 2024-08-16T18:01:33Z

Introduces the BindingPattern and SymbolicBindingPattern insts, and a separate stack of pattern blocks that they are emitted into. The intent is to generate the corresponding pattern-matching insts (like BindName) from them in a separate pass, but that is deferred to future PRs.

See here for the design this is based on, but note that during review we have chosen to deviate from that design by putting the patterns in separate blocks, and omitting the "forward references" from a BindingPattern to its corresponding BindName. This in turn necessitates having separate inst kinds for symbolic and non-symbolic binding patterns.

jonmeow

To be sure I understand correctly, is this change supposed to only be adding BindingPattern without really using it? Can you please use the PR description to elaborate a little on what's being implemented here?

toolchain/sem_ir/inst_namer.cpp

toolchain/sem_ir/inst_kind.def

toolchain/sem_ir/function.cpp

toolchain/check/eval.cpp

toolchain/check/handle_pattern_list.cpp

toolchain/sem_ir/formatter.cpp

toolchain/check/testdata/class/generic/fail_todo_use.carbon

toolchain/check/import_ref.cpp

toolchain/check/handle_binding_pattern.cpp

toolchain/lower/file_context.cpp

toolchain/sem_ir/typed_insts.h

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

geoffromer

To be sure I understand correctly, is this change supposed to only be adding BindingPattern without really using it?

This PR "really uses it" in the way that it's designed to be used. In particular, we traverse the BindingPatterns in order to emit the corresponding BindNames.

Can you please use the PR description to elaborate a little on what's being implemented here?

How's this?

toolchain/check/eval.cpp

toolchain/check/handle_binding_pattern.cpp

toolchain/check/import_ref.cpp

toolchain/lower/file_context.cpp

toolchain/lower/handle.cpp

toolchain/sem_ir/function.cpp

toolchain/sem_ir/inst_kind.def

toolchain/check/testdata/class/generic/fail_todo_use.carbon

toolchain/sem_ir/typed_insts.h

toolchain/check/handle_pattern_list.cpp

To keep things simple, this also rolls back the changes to use BindingPattern to generate the match IR. That will be added in future changes.

geoffromer

I'm particularly looking for feedback on the textual IR changes, because I'm not familiar enough with the conventions to tell if what I have is good, or how to improve them if not.

toolchain/check/handle_binding_pattern.cpp

geoffromer · 2024-09-11T17:40:45Z

toolchain/sem_ir/inst_namer.cpp

+      case SymbolicBindingPattern::Kind: {
+        auto inst = untyped_inst.As<AnyBindingPattern>();
+        add_inst_name_id(
+            sem_ir_.entity_names().Get(inst.entity_name_id).name_id, ".patt");


Note: the ".patt" never appears in the textual IR output. The only effect of omitting that parameter here is that the instruction numbering changes slightly. I can't tell whether any of that is working as intended.

@looking at an example, I think this is related to:

%.loc11_8: i32 = binding_pattern a

I would expect that should be named. My guess would be that the block isn't getting touched. This is probably worth some debugging about why the assignment isn't taking, e.g. by figuring out one of the InstIds for a BindingPattern and then looking at naming calls related to it. (I'd probably just insert logs in the get and set code for starters, maybe logging for every BindingPattern instruction in a test where there's only one)

Do you want help figuring this out, or is this something you'd like to figure out what's going on?

jonmeow

FYI, I'm still looking at this today, but wanted to publish comments in case you want to discuss something during the toolchain meeting. Overall I think the direction this PR is taking seems a good start, though there are still some details that may be good to discuss (particularly see DeclInfo).

jonmeow · 2024-09-17T16:18:37Z

toolchain/check/context.cpp

@@ -147,6 +148,13 @@ auto Context::AddPlaceholderInst(SemIR::LocIdAndInst loc_id_and_inst)
  return inst_id;
 }

+auto Context::AddPatternInst(SemIR::LocIdAndInst loc_id_and_inst)
+    -> SemIR::InstId {
+  auto inst_id = sem_ir().insts().AddInNoBlock(loc_id_and_inst);


Had you considered calling either Context::AddInstInNoBlock or Context::AddPlaceholderInstInNoBlock here? Note both of those provide a little extra logic (for VLOG and constant), which is why they're called from AddInst/AddPlaceholderInst. Normally we'd want a constant associated, which would be AddInstInNoBlock.

jonmeow · 2024-09-17T16:22:58Z

toolchain/check/handle_alias.cpp

+    -> bool {
+  // TODO: when/if parameterized aliases are supported, this must be
+  // attached to the `BindAlias` inst.
+  (void)context.pattern_block_stack().Pop();


Suggested change

(void)context.pattern_block_stack().Pop();

context.pattern_block_stack().PopAndDiscard();

Generally I don't think we write (void) except where required by the compiler for an unused variable/parameter. If you're trying to indicate that there's a value being discarded, maybe the PopAndDiscard call would capture that?

(note, this same comment would apply elsewhere that you're doing this)

jonmeow · 2024-09-17T18:23:07Z

toolchain/sem_ir/typed_insts.h

@@ -302,6 +302,36 @@ struct BindValue {
  InstId value_id;
 };

+// Common representation for various `*binding_pattern` nodes.
+struct AnyBindingPattern {
+  // TODO: Also handle BindTemplateName once it exists.


Should this TODO be updated?

jonmeow · 2024-09-17T18:29:45Z

toolchain/sem_ir/inst_namer.cpp

+      case SymbolicBindingPattern::Kind: {
+        auto inst = untyped_inst.As<AnyBindingPattern>();
+        add_inst_name_id(
+            sem_ir_.entity_names().Get(inst.entity_name_id).name_id, ".patt");


@looking at an example, I think this is related to:

%.loc11_8: i32 = binding_pattern a

I would expect that should be named. My guess would be that the block isn't getting touched. This is probably worth some debugging about why the assignment isn't taking, e.g. by figuring out one of the InstIds for a BindingPattern and then looking at naming calls related to it. (I'd probably just insert logs in the get and set code for starters, maybe logging for every BindingPattern instruction in a test where there's only one)

Do you want help figuring this out, or is this something you'd like to figure out what's going on?

jonmeow · 2024-09-17T18:31:26Z

toolchain/sem_ir/inst_namer.cpp

+        DeclInfo decl = sem_ir_.decls().Get(inst.decl_id);
+        CollectNamesInBlock(scope_id, decl.pattern_block_id);
+        CollectNamesInBlock(scope_id, decl.decl_block_id);


Looking at the copies, had you considered a helper function for this?

jonmeow · 2024-09-17T18:48:03Z

toolchain/sem_ir/decl.h

+// Information about a declaration.
+struct DeclInfo {
+  // The pattern block, containing the pattern insts for the parameter pattern.
+  InstBlockId pattern_block_id;
+  // The declaration block, containing the declaration's parameters and their
+  // types.
+  InstBlockId decl_block_id;
+};


What had you considered as alternatives before the DeclInfo approach? A couple I'm wondering about are:

Adding an instruction to decl_block_id which just declares that there's a pattern

e.g. struct PatternBlock { InstBlockId block_id; }

i.e., as a generic approach that could apply to any block that adds a pattern.

Putting pattern_block_id on EntityWithParams

i.e., tracking it alongside parameters that it's providing a pattern for

To explain why I'm thinking about this, adding more ValueStores feels a little incremental complexity for understanding the toolchain. decl_block_id was added to instructions because it fit into the instruction and was mainly for formatting. However, as part of splitting out to a new structure, it's no longer taking advantage of free space in the declaration's instruction.

Maybe it'd help for me to understand, what's the access pattern you expect for pattern_block_id?

If only kept for the printed IR, the PatternBlock instruction approach might be good: it keeps an association that you could get formatted instructions for, and also provides an approach that would work for arbitrary constructs. The overhead is marginally higher (essentially 12 more bytes), but more flexible beyond the current set of declarations. I expect this approach would need the PatternBlock to be skipped for lowering, but it'd just be the one instruction.

If you need to go back to the pattern block from the decl instruction later during checking, then EntityWithParams might be good: we would probably need it to be associated with the params themselves. EntityWithParams is already shared where you're using DeclInfo, meaning overhead is marginally lower (essentially 4 fewer bytes).

jonmeow

Actually, I guess I was almost done looking through, just a couple more comments.

jonmeow · 2024-09-17T20:14:42Z

toolchain/check/handle_class.cpp

@@ -41,6 +41,8 @@ auto HandleParseNode(Context& context, Parse::ClassIntroducerId node_id)
  context.decl_name_stack().PushScopeAndStartName();
  // This class is potentially generic.
  StartGenericDecl(context);
+  // Start a new pattern block for the signature.
+  context.pattern_block_stack().Push();


Just a note, but something else to consider with storage might be whether we want the parameters and implicit parameters to each have their own pattern block. A single block is fine for now, I'm not asking for a change in this PR, but it might be something that affects the implementation more later.

jonmeow · 2024-09-17T20:21:26Z

toolchain/check/pattern_block_stack.h

+class Context;
+
+// Stack of blocks containing pattern insts.
+class PatternBlockStack {


Had you reconsidered reusing InstBlockStack? There's a lot of logic there that might apply here too: avoiding allocation for empty blocks, debug logging, printing support for stack traces, and so on.

geoffromer added 3 commits August 16, 2024 10:39

binding_pattern in let

99ac089

Checkpoint: tuple pattern support

585d773

Fix handling of params, and inst formatting

45e1ebd

github-actions bot added the toolchain label Aug 16, 2024

github-actions bot requested a review from josh11b August 16, 2024 18:01

geoffromer marked this pull request as draft August 16, 2024 18:05

geoffromer marked this pull request as ready for review August 16, 2024 18:07

github-actions bot requested a review from jonmeow August 16, 2024 18:07

geoffromer removed the request for review from josh11b August 16, 2024 18:08

Fix clang-tidy

c7cbe2e

jonmeow reviewed Aug 16, 2024

View reviewed changes

toolchain/sem_ir/typed_insts.h Show resolved Hide resolved

geoffromer and others added 3 commits August 19, 2024 14:02

Apply suggestions from code review

98130ce

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

Respond to reviewer comments

d1104e0

Respond to reviewer comments

b07e5a6

geoffromer commented Aug 20, 2024

View reviewed changes

geoffromer added 6 commits August 20, 2024 17:23

Respond to reviewer comments

53316c2

Record better location for BindingPattern inst

591fc76

Revert unintended changes in let handling

8242f56

Move pattern insts to separate block

81eed2d

Roll back obsolete lowering changes

7585f55

Drop "forward references" in BindingPattern

bee1dcf

To keep things simple, this also rolls back the changes to use BindingPattern to generate the match IR. That will be added in future changes.

geoffromer commented Sep 11, 2024

View reviewed changes

geoffromer requested a review from jonmeow September 13, 2024 20:51

jonmeow reviewed Sep 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for binding patterns in SemIR #4221

Initial support for binding patterns in SemIR #4221

geoffromer commented Aug 16, 2024 •

edited

Loading

jonmeow left a comment

geoffromer left a comment

geoffromer left a comment

geoffromer Sep 11, 2024

jonmeow Sep 17, 2024

jonmeow left a comment

jonmeow Sep 17, 2024

jonmeow Sep 17, 2024

jonmeow Sep 17, 2024

jonmeow Sep 17, 2024

jonmeow Sep 17, 2024

jonmeow Sep 17, 2024

jonmeow left a comment

jonmeow Sep 17, 2024

jonmeow Sep 17, 2024

	(void)context.pattern_block_stack().Pop();
	context.pattern_block_stack().PopAndDiscard();

Initial support for binding patterns in SemIR #4221

Are you sure you want to change the base?

Initial support for binding patterns in SemIR #4221

Conversation

geoffromer commented Aug 16, 2024 • edited Loading

jonmeow left a comment

Choose a reason for hiding this comment

geoffromer left a 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

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

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 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 Aug 16, 2024 •

edited

Loading