From 71c91eaf14eb25dde6a12d4250090b526dc988f0 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 12 Feb 2025 11:14:37 -0800 Subject: [PATCH] Refactor subpattern logic out of Context (#4929) Note this is building on #4927 which factors out RegionStack, because this heavily uses the region stack. However, it's a more complex level of logic that appears specific to pattern handling. I've moved InsertHere to pattern_match.cpp because it appears to be in specific use there. --- toolchain/check/BUILD | 2 + toolchain/check/context.cpp | 72 ---------------------- toolchain/check/context.h | 26 -------- toolchain/check/handle_binding_pattern.cpp | 3 +- toolchain/check/handle_let_and_var.cpp | 5 +- toolchain/check/handle_pattern_list.cpp | 11 ++-- toolchain/check/pattern_match.cpp | 49 ++++++++++++++- toolchain/check/subpattern.cpp | 43 +++++++++++++ toolchain/check/subpattern.h | 34 ++++++++++ 9 files changed, 138 insertions(+), 107 deletions(-) create mode 100644 toolchain/check/subpattern.cpp create mode 100644 toolchain/check/subpattern.h diff --git a/toolchain/check/BUILD b/toolchain/check/BUILD index 39eb89ff74ea..a2f7f4c10227 100644 --- a/toolchain/check/BUILD +++ b/toolchain/check/BUILD @@ -39,6 +39,7 @@ cc_library( "operator.cpp", "pattern_match.cpp", "return.cpp", + "subpattern.cpp", "subst.cpp", "type_completion.cpp", ], @@ -73,6 +74,7 @@ cc_library( "pending_block.h", "region_stack.h", "return.h", + "subpattern.h", "subst.h", "type_completion.h", ], diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 21740b6a38b9..c83aed6c3b2d 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -747,78 +747,6 @@ auto Context::LookupNameInCore(SemIR::LocId loc_id, llvm::StringRef name) return constant_values().GetConstantInstId(scope_result.target_inst_id()); } -auto Context::BeginSubpattern() -> void { - inst_block_stack().Push(); - region_stack_.PushRegion(inst_block_stack().PeekOrAdd()); -} - -auto Context::EndSubpatternAsExpr(SemIR::InstId result_id) - -> SemIR::ExprRegionId { - if (region_stack_.PeekRegion().size() > 1) { - // End the exit block with a branch to a successor block, whose contents - // will be determined later. - AddInst(SemIR::LocIdAndInst::NoLoc( - {.target_id = inst_blocks().AddDefaultValue()})); - } else { - // This single-block region will be inserted as a SpliceBlock, so we don't - // need control flow out of it. - } - auto block_id = inst_block_stack().Pop(); - CARBON_CHECK(block_id == region_stack_.PeekRegion().back()); - - // TODO: Is it possible to validate that this region is genuinely - // single-entry, single-exit? - return sem_ir().expr_regions().Add( - {.block_ids = region_stack_.PopRegion(), .result_id = result_id}); -} - -auto Context::EndSubpatternAsEmpty() -> void { - auto block_id = inst_block_stack().Pop(); - CARBON_CHECK(block_id == region_stack_.PeekRegion().back()); - CARBON_CHECK(region_stack_.PeekRegion().size() == 1); - CARBON_CHECK(inst_blocks().Get(block_id).empty()); - region_stack_.PopAndDiscardRegion(); -} - -auto Context::InsertHere(SemIR::ExprRegionId region_id) -> SemIR::InstId { - auto region = sem_ir_->expr_regions().Get(region_id); - auto loc_id = insts().GetLocId(region.result_id); - auto exit_block = inst_blocks().Get(region.block_ids.back()); - if (region.block_ids.size() == 1) { - // TODO: Is it possible to avoid leaving an "orphan" block in the IR in the - // first two cases? - if (exit_block.empty()) { - return region.result_id; - } - if (exit_block.size() == 1) { - inst_block_stack_.AddInstId(exit_block.front()); - return region.result_id; - } - return AddInst( - loc_id, {.type_id = insts().Get(region.result_id).type_id(), - .block_id = region.block_ids.front(), - .result_id = region.result_id}); - } - if (region_stack_.empty()) { - TODO(loc_id, - "Control flow expressions are currently only supported inside " - "functions."); - return SemIR::ErrorInst::SingletonInstId; - } - AddInst(SemIR::LocIdAndInst::NoLoc( - {.target_id = region.block_ids.front()})); - inst_block_stack_.Pop(); - // TODO: this will cumulatively cost O(MN) running time for M blocks - // at the Nth level of the stack. Figure out how to do better. - region_stack_.AddToRegion(region.block_ids); - auto resume_with_block_id = - insts().GetAs(exit_block.back()).target_id; - CARBON_CHECK(inst_blocks().GetOrEmpty(resume_with_block_id).empty()); - inst_block_stack_.Push(resume_with_block_id); - region_stack_.AddToRegion(resume_with_block_id, loc_id); - return region.result_id; -} - auto Context::Finalize() -> void { // Pop information for the file-level scope. sem_ir().set_top_inst_block_id(inst_block_stack().Pop()); diff --git a/toolchain/check/context.h b/toolchain/check/context.h index b2142c11a5b9..39c5741ff439 100644 --- a/toolchain/check/context.h +++ b/toolchain/check/context.h @@ -543,32 +543,6 @@ class Context { auto global_init() -> GlobalInit& { return global_init_; } - // Marks the start of a region of insts in a pattern context that might - // represent an expression or a pattern. Typically this is called when - // handling a parse node that can immediately precede a subpattern (such - // as `let` or a `,` in a pattern list), and the handler for the subpattern - // node makes the matching `EndSubpatternAs*` call. - auto BeginSubpattern() -> void; - - // Ends a region started by BeginSubpattern (in stack order), treating it as - // an expression with the given result, and returns the ID of the region. The - // region will not yet have any control-flow edges into or out of it. - auto EndSubpatternAsExpr(SemIR::InstId result_id) -> SemIR::ExprRegionId; - - // Ends a region started by BeginSubpattern (in stack order), asserting that - // it was empty. - auto EndSubpatternAsEmpty() -> void; - - // TODO: Add EndSubpatternAsPattern, when needed. - - // Inserts the given region into the current code block. If the region - // consists of a single block, this will be implemented as a `splice_block` - // inst. Otherwise, this will end the current block with a branch to the entry - // block of the region, and add future insts to a new block which is the - // immediate successor of the region's exit block. As a result, this cannot be - // called more than once for the same region. - auto InsertHere(SemIR::ExprRegionId region_id) -> SemIR::InstId; - auto import_ref_ids() -> llvm::SmallVector& { return import_ref_ids_; } diff --git a/toolchain/check/handle_binding_pattern.cpp b/toolchain/check/handle_binding_pattern.cpp index 25004ac3b8bc..04ce50c0d7a5 100644 --- a/toolchain/check/handle_binding_pattern.cpp +++ b/toolchain/check/handle_binding_pattern.cpp @@ -7,6 +7,7 @@ #include "toolchain/check/handle.h" #include "toolchain/check/interface.h" #include "toolchain/check/return.h" +#include "toolchain/check/subpattern.h" #include "toolchain/check/type_completion.h" #include "toolchain/diagnostics/format_providers.h" #include "toolchain/sem_ir/ids.h" @@ -24,7 +25,7 @@ static auto HandleAnyBindingPattern(Context& context, Parse::NodeId node_id, // TODO: Handle `_` bindings. SemIR::ExprRegionId type_expr_region_id = - context.EndSubpatternAsExpr(cast_type_inst_id); + EndSubpatternAsExpr(context, cast_type_inst_id); // Every other kind of pattern binding has a name. auto [name_node, name_id] = context.node_stack().PopNameWithNodeId(); diff --git a/toolchain/check/handle_let_and_var.cpp b/toolchain/check/handle_let_and_var.cpp index d04076b3a41e..742d0dd53b01 100644 --- a/toolchain/check/handle_let_and_var.cpp +++ b/toolchain/check/handle_let_and_var.cpp @@ -12,6 +12,7 @@ #include "toolchain/check/modifiers.h" #include "toolchain/check/pattern_match.h" #include "toolchain/check/return.h" +#include "toolchain/check/subpattern.h" #include "toolchain/diagnostics/diagnostic_emitter.h" #include "toolchain/lex/token_kind.h" #include "toolchain/parse/node_kind.h" @@ -82,7 +83,7 @@ static auto HandleIntroducer(Context& context, Parse::NodeId node_id) -> bool { context.pattern_block_stack().Push(); context.full_pattern_stack().PushFullPattern( FullPatternStack::Kind::NameBindingDecl); - context.BeginSubpattern(); + BeginSubpattern(context); return true; } @@ -256,7 +257,7 @@ static auto HandleDecl(Context& context) -> DeclInfo { // associated with an InstBlockId on the node stack rather than an InstId, // and leave behind an entry on the subpattern stack and one on the node // stack. - context.EndSubpatternAsExpr(SemIR::ErrorInst::SingletonInstId); + EndSubpatternAsExpr(context, SemIR::ErrorInst::SingletonInstId); context.node_stack().PopForSoloNodeId(); decl_info.pattern_id = SemIR::ErrorInst::SingletonInstId; } else { diff --git a/toolchain/check/handle_pattern_list.cpp b/toolchain/check/handle_pattern_list.cpp index ffee24c3c5d7..7e80e6434f4a 100644 --- a/toolchain/check/handle_pattern_list.cpp +++ b/toolchain/check/handle_pattern_list.cpp @@ -4,6 +4,7 @@ #include "toolchain/check/context.h" #include "toolchain/check/handle.h" +#include "toolchain/check/subpattern.h" namespace Carbon::Check { @@ -11,7 +12,7 @@ auto HandleParseNode(Context& context, Parse::ImplicitParamListStartId node_id) -> bool { context.node_stack().Push(node_id); context.param_and_arg_refs_stack().Push(); - context.BeginSubpattern(); + BeginSubpattern(context); return true; } @@ -20,7 +21,7 @@ auto HandleParseNode(Context& context, Parse::ImplicitParamListId node_id) if (context.node_stack().PeekIs(Parse::NodeKind::ImplicitParamListStart)) { // End the subpattern started by a trailing comma, or the opening delimiter // of an empty list. - context.EndSubpatternAsEmpty(); + EndSubpatternAsEmpty(context); } // Note the Start node remains on the stack, where the param list handler can // make use of it. @@ -36,7 +37,7 @@ auto HandleParseNode(Context& context, Parse::TuplePatternStartId node_id) -> bool { context.node_stack().Push(node_id); context.param_and_arg_refs_stack().Push(); - context.BeginSubpattern(); + BeginSubpattern(context); // TODO: Remove this branch once the parse tree differentiates between // tuple patterns and param patterns. if (context.full_pattern_stack().CurrentKind() == @@ -49,7 +50,7 @@ auto HandleParseNode(Context& context, Parse::TuplePatternStartId node_id) auto HandleParseNode(Context& context, Parse::PatternListCommaId /*node_id*/) -> bool { context.param_and_arg_refs_stack().ApplyComma(); - context.BeginSubpattern(); + BeginSubpattern(context); return true; } @@ -57,7 +58,7 @@ auto HandleParseNode(Context& context, Parse::TuplePatternId node_id) -> bool { if (context.node_stack().PeekIs(Parse::NodeKind::TuplePatternStart)) { // End the subpattern started by a trailing comma, or the opening delimiter // of an empty list. - context.EndSubpatternAsEmpty(); + EndSubpatternAsEmpty(context); } // Note the Start node remains on the stack, where the param list handler can // make use of it. diff --git a/toolchain/check/pattern_match.cpp b/toolchain/check/pattern_match.cpp index a3770375161d..e88264c9456c 100644 --- a/toolchain/check/pattern_match.cpp +++ b/toolchain/check/pattern_match.cpp @@ -12,6 +12,7 @@ #include "toolchain/base/kind_switch.h" #include "toolchain/check/context.h" #include "toolchain/check/convert.h" +#include "toolchain/check/subpattern.h" namespace Carbon::Check { @@ -124,6 +125,52 @@ auto MatchContext::DoWork(Context& context) -> SemIR::InstBlockId { return block_id; } +// Inserts the given region into the current code block. If the region +// consists of a single block, this will be implemented as a `splice_block` +// inst. Otherwise, this will end the current block with a branch to the entry +// block of the region, and add future insts to a new block which is the +// immediate successor of the region's exit block. As a result, this cannot be +// called more than once for the same region. +static auto InsertHere(Context& context, SemIR::ExprRegionId region_id) + -> SemIR::InstId { + auto region = context.sem_ir().expr_regions().Get(region_id); + auto loc_id = context.insts().GetLocId(region.result_id); + auto exit_block = context.inst_blocks().Get(region.block_ids.back()); + if (region.block_ids.size() == 1) { + // TODO: Is it possible to avoid leaving an "orphan" block in the IR in the + // first two cases? + if (exit_block.empty()) { + return region.result_id; + } + if (exit_block.size() == 1) { + context.inst_block_stack().AddInstId(exit_block.front()); + return region.result_id; + } + return context.AddInst( + loc_id, {.type_id = context.insts().Get(region.result_id).type_id(), + .block_id = region.block_ids.front(), + .result_id = region.result_id}); + } + if (context.region_stack().empty()) { + context.TODO(loc_id, + "Control flow expressions are currently only supported inside " + "functions."); + return SemIR::ErrorInst::SingletonInstId; + } + context.AddInst(SemIR::LocIdAndInst::NoLoc( + {.target_id = region.block_ids.front()})); + context.inst_block_stack().Pop(); + // TODO: this will cumulatively cost O(MN) running time for M blocks + // at the Nth level of the stack. Figure out how to do better. + context.region_stack().AddToRegion(region.block_ids); + auto resume_with_block_id = + context.insts().GetAs(exit_block.back()).target_id; + CARBON_CHECK(context.inst_blocks().GetOrEmpty(resume_with_block_id).empty()); + context.inst_block_stack().Push(resume_with_block_id); + context.region_stack().AddToRegion(resume_with_block_id, loc_id); + return region.result_id; +} + auto MatchContext::EmitPatternMatch(Context& context, MatchContext::WorkItem entry) -> void { if (entry.pattern_id == SemIR::ErrorInst::SingletonInstId) { @@ -149,7 +196,7 @@ auto MatchContext::EmitPatternMatch(Context& context, context.bind_name_map().Lookup(entry.pattern_id).value(), {.bind_name_id = SemIR::InstId::None, .type_expr_region_id = SemIR::ExprRegionId::None}); - context.InsertHere(type_expr_region_id); + InsertHere(context, type_expr_region_id); auto value_id = entry.scrutinee_id; switch (kind_) { case MatchKind::Local: { diff --git a/toolchain/check/subpattern.cpp b/toolchain/check/subpattern.cpp new file mode 100644 index 000000000000..e49b418fa91e --- /dev/null +++ b/toolchain/check/subpattern.cpp @@ -0,0 +1,43 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include "toolchain/check/subpattern.h" + +namespace Carbon::Check { + +auto BeginSubpattern(Context& context) -> void { + context.inst_block_stack().Push(); + context.region_stack().PushRegion(context.inst_block_stack().PeekOrAdd()); +} + +auto EndSubpatternAsExpr(Context& context, SemIR::InstId result_id) + -> SemIR::ExprRegionId { + if (context.region_stack().PeekRegion().size() > 1) { + // End the exit block with a branch to a successor block, whose contents + // will be determined later. + context.AddInst(SemIR::LocIdAndInst::NoLoc( + {.target_id = context.inst_blocks().AddDefaultValue()})); + } else { + // This single-block region will be inserted as a SpliceBlock, so we don't + // need control flow out of it. + } + auto block_id = context.inst_block_stack().Pop(); + CARBON_CHECK(block_id == context.region_stack().PeekRegion().back()); + + // TODO: Is it possible to validate that this region is genuinely + // single-entry, single-exit? + return context.sem_ir().expr_regions().Add( + {.block_ids = context.region_stack().PopRegion(), + .result_id = result_id}); +} + +auto EndSubpatternAsEmpty(Context& context) -> void { + auto block_id = context.inst_block_stack().Pop(); + CARBON_CHECK(block_id == context.region_stack().PeekRegion().back()); + CARBON_CHECK(context.region_stack().PeekRegion().size() == 1); + CARBON_CHECK(context.inst_blocks().Get(block_id).empty()); + context.region_stack().PopAndDiscardRegion(); +} + +} // namespace Carbon::Check diff --git a/toolchain/check/subpattern.h b/toolchain/check/subpattern.h new file mode 100644 index 000000000000..43856fa00e8a --- /dev/null +++ b/toolchain/check/subpattern.h @@ -0,0 +1,34 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#ifndef CARBON_TOOLCHAIN_CHECK_SUBPATTERN_H_ +#define CARBON_TOOLCHAIN_CHECK_SUBPATTERN_H_ + +#include "toolchain/check/context.h" +#include "toolchain/sem_ir/ids.h" + +namespace Carbon::Check { + +// Marks the start of a region of insts in a pattern context that might +// represent an expression or a pattern. Typically this is called when +// handling a parse node that can immediately precede a subpattern (such +// as `let` or a `,` in a pattern list), and the handler for the subpattern +// node makes the matching `EndSubpatternAs*` call. +auto BeginSubpattern(Context& context) -> void; + +// Ends a region started by BeginSubpattern (in stack order), treating it as +// an expression with the given result, and returns the ID of the region. The +// region will not yet have any control-flow edges into or out of it. +auto EndSubpatternAsExpr(Context& context, SemIR::InstId result_id) + -> SemIR::ExprRegionId; + +// Ends a region started by BeginSubpattern (in stack order), asserting that +// it was empty. +auto EndSubpatternAsEmpty(Context& context) -> void; + +// TODO: Add EndSubpatternAsPattern, when needed. + +} // namespace Carbon::Check + +#endif // CARBON_TOOLCHAIN_CHECK_SUBPATTERN_H_