From e3764ff6f3365e2ecc550f5bf7f41d4a9965702b Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 14 Feb 2025 17:18:54 -0800 Subject: [PATCH] Clean up Context API (#4969) - Add a better class comment. - Remove the obsolete `type_ids_for_type_constants_` and `TypeNode`. - For `bind_name_map`, fix declaration order and move comment to be consistent with other members. - Reorder accessors to better match order of data members. - This is how I noticed `type_ids_for_type_constants_` - Put a bigger notice so that `sem_ir` helper functions don't end up in the middle of other members again. --- toolchain/check/context.cpp | 9 --- toolchain/check/context.h | 123 +++++++++++++++++------------------- 2 files changed, 59 insertions(+), 73 deletions(-) diff --git a/toolchain/check/context.cpp b/toolchain/check/context.cpp index 607954a3bed2..11970f452e5e 100644 --- a/toolchain/check/context.cpp +++ b/toolchain/check/context.cpp @@ -62,15 +62,6 @@ Context::Context(DiagnosticEmitter* emitter, import_ir_constant_values_.reserve(imported_ir_count); check_ir_map_.resize(total_ir_count, SemIR::ImportIRId::None); - // Map the builtin `` and `type` type constants to their corresponding - // special `TypeId` values. - type_ids_for_type_constants_.Insert( - SemIR::ConstantId::ForConcreteConstant(SemIR::ErrorInst::SingletonInstId), - SemIR::ErrorInst::SingletonTypeId); - type_ids_for_type_constants_.Insert( - SemIR::ConstantId::ForConcreteConstant(SemIR::TypeType::SingletonInstId), - SemIR::TypeType::SingletonTypeId); - // TODO: Remove this and add a `VerifyOnFinish` once we properly push and pop // in the right places. generic_region_stack().Push(); diff --git a/toolchain/check/context.h b/toolchain/check/context.h index 9d60c9278bb3..5bf3c5a89737 100644 --- a/toolchain/check/context.h +++ b/toolchain/check/context.h @@ -32,7 +32,19 @@ namespace Carbon::Check { -// Context and shared functionality for semantics handlers. +// Context stored during check. +// +// This file stores state, and members objects may provide an API. Other files +// may also have helpers that operate on Context. To keep this file manageable, +// please put logic into other files. +// +// For example, consider the API for functions: +// - `context.functions()`: Exposes storage of `SemIR::Function` objects. +// - `toolchain/check/function.h`: Contains helper functions which use +// `Check::Context`. +// - `toolchain/sem_ir/function.h`: Contains helper functions which only need +// `SemIR` objects, for which it's helpful not to depend on `Check::Context` +// (for example, shared with lowering). class Context { public: using DiagnosticEmitter = Carbon::DiagnosticEmitter; @@ -43,6 +55,14 @@ class Context { // add contextual notes as appropriate. using BuildDiagnosticFn = llvm::function_refDiagnosticBuilder>; + // Pre-computed parts of a binding pattern. + struct BindingPatternInfo { + // The corresponding AnyBindName inst. + SemIR::InstId bind_name_id; + // The region of insts that computes the type of the binding. + SemIR::ExprRegionId type_expr_region_id; + }; + // An ongoing impl lookup, used to ensure termination. struct ImplLookupStackEntry { SemIR::ConstantId type_const_id; @@ -86,10 +106,10 @@ class Context { auto sem_ir() -> SemIR::File& { return *sem_ir_; } auto sem_ir() const -> const SemIR::File& { return *sem_ir_; } + // Convenience functions for major phase data. auto parse_tree() const -> const Parse::Tree& { return sem_ir_->parse_tree(); } - auto tokens() const -> const Lex::TokenizedBuffer& { return parse_tree().tokens(); } @@ -125,14 +145,17 @@ class Context { auto scope_stack() -> ScopeStack& { return scope_stack_; } + // Conveneicne functions for frequently-used `scope_stack` members. auto return_scope_stack() -> llvm::SmallVector& { return scope_stack().return_scope_stack(); } - auto break_continue_stack() -> llvm::SmallVector& { return scope_stack().break_continue_stack(); } + auto full_pattern_stack() -> FullPatternStack& { + return scope_stack_.full_pattern_stack(); + } auto generic_region_stack() -> GenericRegionStack& { return generic_region_stack_; @@ -149,7 +172,35 @@ class Context { return import_ir_constant_values_; } + auto definitions_required() -> llvm::SmallVector& { + return definitions_required_; + } + + auto global_init() -> GlobalInit& { return global_init_; } + + auto import_ref_ids() -> llvm::SmallVector& { + return import_ref_ids_; + } + + // TODO: Consider putting this behind a narrower API to guard against emitting + // multiple times. + auto bind_name_map() -> Map& { + return bind_name_map_; + } + + auto var_storage_map() -> Map& { + return var_storage_map_; + } + + auto region_stack() -> RegionStack& { return region_stack_; } + + auto impl_lookup_stack() -> llvm::SmallVector& { + return impl_lookup_stack_; + } + + // -------------------------------------------------------------------------- // Directly expose SemIR::File data accessors for brevity in calls. + // -------------------------------------------------------------------------- auto identifiers() -> SharedValueStores::IdentifierStore& { return sem_ir().identifiers(); @@ -207,59 +258,11 @@ class Context { } auto constants() -> SemIR::ConstantStore& { return sem_ir().constants(); } - auto definitions_required() -> llvm::SmallVector& { - return definitions_required_; - } - - auto global_init() -> GlobalInit& { return global_init_; } - - auto import_ref_ids() -> llvm::SmallVector& { - return import_ref_ids_; - } - - // Map from an AnyBindingPattern inst to precomputed parts of the - // pattern-match SemIR for it. - // - // TODO: Consider putting this behind a narrower API to guard against emitting - // multiple times. - struct BindingPatternInfo { - // The corresponding AnyBindName inst. - SemIR::InstId bind_name_id; - // The region of insts that computes the type of the binding. - SemIR::ExprRegionId type_expr_region_id; - }; - auto bind_name_map() -> Map& { - return bind_name_map_; - } - - auto var_storage_map() -> Map& { - return var_storage_map_; - } - - auto region_stack() -> RegionStack& { return region_stack_; } - - auto full_pattern_stack() -> FullPatternStack& { - return scope_stack_.full_pattern_stack(); - } - - auto impl_lookup_stack() -> llvm::SmallVector& { - return impl_lookup_stack_; - } + // -------------------------------------------------------------------------- + // End of SemIR::File members. + // -------------------------------------------------------------------------- private: - // A FoldingSet node for a type. - class TypeNode : public llvm::FastFoldingSetNode { - public: - explicit TypeNode(const llvm::FoldingSetNodeID& node_id, - SemIR::TypeId type_id) - : llvm::FastFoldingSetNode(node_id), type_id_(type_id) {} - - auto type_id() -> SemIR::TypeId { return type_id_; } - - private: - SemIR::TypeId type_id_; - }; - // Handles diagnostics. DiagnosticEmitter* emitter_; @@ -313,16 +316,6 @@ class Context { // defined, regardless of whether the class can have virtual functions. InstBlockStack vtable_stack_; - // Cache of reverse mapping from type constants to types. - // - // TODO: Instead of mapping to a dense `TypeId` space, we could make `TypeId` - // be a thin wrapper around `ConstantId` and only perform the lookup only when - // we want to access the completeness and value representation of a type. It's - // not clear whether that would result in more or fewer lookups. - // - // TODO: Should this be part of the `TypeStore`? - Map type_ids_for_type_constants_; - // The list which will form NodeBlockId::Exports. llvm::SmallVector exports_; @@ -351,6 +344,8 @@ class Context { // FinalizeImportRefBlock() will produce an inst block for them. llvm::SmallVector import_ref_ids_; + // Map from an AnyBindingPattern inst to precomputed parts of the + // pattern-match SemIR for it. Map bind_name_map_; // Map from VarPattern insts to the corresponding VarStorage insts. The