From f747cb0c9335492e5a6dbfe7cb5bdb104c64fb51 Mon Sep 17 00:00:00 2001 From: Geoff Romer Date: Tue, 19 Sep 2023 13:32:48 -0700 Subject: [PATCH] Add cache of template instantiations. (#3243) Also some drive-by comment and trace improvements. Closes #2951 --- explorer/interpreter/interpreter.cpp | 31 ++++++++++---- explorer/interpreter/type_checker.cpp | 41 ++++++++++++------- explorer/interpreter/type_checker.h | 18 ++++++-- .../template/instantiations_are_cached.carbon | 32 +++++++++++++++ .../testdata/trace/phase_execution.carbon | 3 ++ 5 files changed, 98 insertions(+), 27 deletions(-) create mode 100644 explorer/testdata/template/instantiations_are_cached.carbon diff --git a/explorer/interpreter/interpreter.cpp b/explorer/interpreter/interpreter.cpp index 2fb3231c95c8..bd3d057953d3 100644 --- a/explorer/interpreter/interpreter.cpp +++ b/explorer/interpreter/interpreter.cpp @@ -516,49 +516,62 @@ auto Interpreter::InstantiateType(Nonnull type, << source_loc << ")\n"; } + const Value* value = nullptr; switch (type->kind()) { case Value::Kind::VariableType: { CARBON_ASSIGN_OR_RETURN( - Nonnull value, + value, todo_.ValueOfNode(&cast(*type).binding(), source_loc)); if (const auto* location = dyn_cast(value)) { CARBON_ASSIGN_OR_RETURN(value, heap_.Read(location->address(), source_loc)); } - return value; + break; } case Value::Kind::InterfaceType: { const auto& interface_type = cast(*type); CARBON_ASSIGN_OR_RETURN( Nonnull bindings, InstantiateBindings(&interface_type.bindings(), source_loc)); - return arena_->New(&interface_type.declaration(), - bindings); + value = + arena_->New(&interface_type.declaration(), bindings); + break; } case Value::Kind::NamedConstraintType: { const auto& constraint_type = cast(*type); CARBON_ASSIGN_OR_RETURN( Nonnull bindings, InstantiateBindings(&constraint_type.bindings(), source_loc)); - return arena_->New(&constraint_type.declaration(), - bindings); + value = arena_->New(&constraint_type.declaration(), + bindings); + break; } case Value::Kind::ChoiceType: { const auto& choice_type = cast(*type); CARBON_ASSIGN_OR_RETURN( Nonnull bindings, InstantiateBindings(&choice_type.bindings(), source_loc)); - return arena_->New(&choice_type.declaration(), bindings); + value = arena_->New(&choice_type.declaration(), bindings); + break; } case Value::Kind::AssociatedConstant: { CARBON_ASSIGN_OR_RETURN( Nonnull type_value, EvalAssociatedConstant(cast(type), source_loc)); - return type_value; + value = type_value; + break; } default: - return type; + value = type; + break; } + + if (trace_stream_->is_enabled()) { + trace_stream_->End() << "instantiated type `" << *type << "` as `" << *value + << "` (" << source_loc << ")\n"; + } + + return value; } auto Interpreter::InstantiateBindings(Nonnull bindings, diff --git a/explorer/interpreter/type_checker.cpp b/explorer/interpreter/type_checker.cpp index ee85317a0ae9..51c750617456 100644 --- a/explorer/interpreter/type_checker.cpp +++ b/explorer/interpreter/type_checker.cpp @@ -2176,13 +2176,10 @@ class TypeChecker::SubstituteTransform const auto* declaration = &witness->declaration(); if (!IsTemplateSaturated(witness->bindings()) && IsTemplateSaturated(*bindings)) { - CARBON_ASSIGN_OR_RETURN( - CARBON_PROTECT_COMMAS(auto [new_decl, new_bindings]), - type_checker_->InstantiateImplDeclaration(declaration, bindings)); - declaration = new_decl; - bindings = new_bindings; + return type_checker_->InstantiateImplDeclaration(declaration, bindings); + } else { + return type_checker_->arena_->New(declaration, bindings); } - return type_checker_->arena_->New(declaration, bindings); } // For an associated constant, look for a rewrite. @@ -5805,6 +5802,11 @@ auto TypeChecker::DeclareImplDeclaration(Nonnull impl_decl, << impl_decl->source_loc() << ")\n"; } + // We need to eagerly typecheck portions of the impl in terms of the generic + // parameters, and then typecheck it again at instantiation time in terms of + // the actual arguments. The AST doesn't allow type information to be mutated, + // So in order to do that, we need to preserve a clone that doesn't have type + // information attached. if (!IsTemplateSaturated(impl_decl->deduced_parameters())) { CloneContext context(arena_); TemplateInfo template_info = {.pattern = context.Clone(impl_decl)}; @@ -6502,25 +6504,31 @@ auto TypeChecker::FindCollectedMembers(Nonnull decl) } auto TypeChecker::InstantiateImplDeclaration( - Nonnull old_impl, + Nonnull pattern, Nonnull bindings) const - -> ErrorOr, Nonnull>> { + -> ErrorOr> { CARBON_CHECK(IsTemplateSaturated(*bindings)); if (trace_stream_->is_enabled()) { - trace_stream_->Start() << "instantiating `" << PrintAsID(*old_impl) << "` (" - << old_impl->source_loc() << ")\n"; + trace_stream_->Start() << "instantiating `" << PrintAsID(*pattern) << "` (" + << pattern->source_loc() << ")\n"; *trace_stream_ << *bindings << "\n"; } - SetFileContext set_file_context(*trace_stream_, old_impl->source_loc()); + SetFileContext set_file_context(*trace_stream_, pattern->source_loc()); - auto it = templates_.find(old_impl); + auto it = templates_.find(pattern); CARBON_CHECK(it != templates_.end()); const TemplateInfo& info = it->second; - // TODO: Only instantiate each declaration once for each set of template - // arguments. + if (auto instantiation = info.instantiations.find(bindings); + instantiation != info.instantiations.end()) { + if (trace_stream_->is_enabled()) { + *trace_stream_ << "reusing cached instantiation\n"; + } + return instantiation->second; + } + CloneContext context(arena_); Nonnull impl = context.Clone(cast(info.pattern)); @@ -6605,7 +6613,10 @@ auto TypeChecker::InstantiateImplDeclaration( /*is_template_instantiation=*/true)); CARBON_RETURN_IF_ERROR(type_checker->TypeCheckImplDeclaration(impl, scope)); - return std::pair{impl, arena_->New(std::move(new_bindings))}; + auto* result = arena_->New( + impl, arena_->New(std::move(new_bindings))); + CARBON_CHECK(info.instantiations.insert({bindings, result}).second); + return result; } auto TypeChecker::InterpExp(Nonnull e) diff --git a/explorer/interpreter/type_checker.h b/explorer/interpreter/type_checker.h index ce52ffa8e9e1..cb5ae26ae581 100644 --- a/explorer/interpreter/type_checker.h +++ b/explorer/interpreter/type_checker.h @@ -558,7 +558,7 @@ class TypeChecker { // template bindings. auto InstantiateImplDeclaration(Nonnull pattern, Nonnull bindings) const - -> ErrorOr, Nonnull>>; + -> ErrorOr>; // Wraps the interpreter's InterpExp, forwarding TypeChecker members as // arguments. @@ -598,8 +598,20 @@ class TypeChecker { // A mapping from the bindings of the type-checked pattern to the bindings // of the original. std::map param_map; - // TODO: Keep track of the instantiations we've already performed and don't - // do them again. + + // Comparator for pointers to Bindings. + struct BindingPtrCompare { + auto operator()(Nonnull lhs, + Nonnull rhs) const { + return std::tie(lhs->args(), lhs->witnesses()) < + std::tie(rhs->args(), rhs->witnesses()); + } + }; + + // Cache of instantiations of this template. + mutable std::map, Nonnull, + BindingPtrCompare> + instantiations; }; // Map from template declarations to extra information we use to type-check diff --git a/explorer/testdata/template/instantiations_are_cached.carbon b/explorer/testdata/template/instantiations_are_cached.carbon new file mode 100644 index 000000000000..f485d3f0648c --- /dev/null +++ b/explorer/testdata/template/instantiations_are_cached.carbon @@ -0,0 +1,32 @@ +// 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 +// +// AUTOUPDATE + +package ExplorerTest api; + +interface I { fn F(); } + +impl () as I { + fn F() {} +} + +impl forall [template T:! type] (T,) as I { + fn F() { + if (true) { + T.(I.F)(); + } else { + T.(I.F)(); + } + } +} + +fn Main() -> i32 { + // This would cause an exponential number of template instantiations if we + // didn't cache them. + ((((((((((((((((((((((((((((((((),),),),),),),),),),),),),),),),),),),),),),),),),),),),),),)).(I.F)(); + return 0; +} + +// CHECK:STDOUT: result: 0 diff --git a/explorer/testdata/trace/phase_execution.carbon b/explorer/testdata/trace/phase_execution.carbon index 5b3e5d6fbf17..d52542d1b95d 100644 --- a/explorer/testdata/trace/phase_execution.carbon +++ b/explorer/testdata/trace/phase_execution.carbon @@ -101,6 +101,8 @@ fn Main() -> i32 { // CHECK:STDOUT: ->> step TypeInstantiationAction pos: 0 `interface As(T = class T)` (phase_execution.carbon:10) ---> // CHECK:STDOUT: ->> instantiating type `interface As(T = class T)` (phase_execution.carbon:10) // CHECK:STDOUT: ->> instantiating type `class T` (phase_execution.carbon:10) +// CHECK:STDOUT: <<- instantiated type `class T` as `class T` (phase_execution.carbon:10) +// CHECK:STDOUT: <<- instantiated type `interface As(T = class T)` as `interface As(T = class T)` (phase_execution.carbon:10) // CHECK:STDOUT: <[] stack-pop: TypeInstantiationAction pos: 0 `interface As(T = class T)` (phase_execution.carbon:10) // CHECK:STDOUT: ->> step ExpressionAction pos: 3 `{}.(interface ImplicitAs(T = class T).Convert)` results: [`{}`, `witness for impl T as ImplicitAs(U)`, `interface As(T = class T)`] (phase_execution.carbon:10) ---> // CHECK:STDOUT: ->> step ExpressionAction pos: 4 `{}.(interface ImplicitAs(T = class T).Convert)` results: [`{}`, `witness for impl T as ImplicitAs(U)`, `interface As(T = class T)`] (phase_execution.carbon:10) ---> @@ -150,6 +152,7 @@ fn Main() -> i32 { // CHECK:STDOUT: <[] stack-pop: TypeInstantiationAction pos: 0 `class T` (prelude.carbon:{{\d+}}) // CHECK:STDOUT: ->> step ExpressionAction pos: 2 `self` results: [`{}`, `class T`] (prelude.carbon:{{\d+}}) ---> // CHECK:STDOUT: ->> instantiating type `class T` (prelude.carbon:{{\d+}}) +// CHECK:STDOUT: <<- instantiated type `class T` as `class T` (prelude.carbon:{{\d+}}) // CHECK:STDOUT: <[] stack-pop: ExpressionAction pos: 2 `self` results: [`{}`, `class T`] (prelude.carbon:{{\d+}}) // CHECK:STDOUT: ->> step ValueExpressionAction pos: 1 `__intrinsic_implicit_as_convert(self, U)` results: [`T{}`] (prelude.carbon:{{\d+}}) ---> // CHECK:STDOUT: <[] stack-pop: ValueExpressionAction pos: 1 `__intrinsic_implicit_as_convert(self, U)` results: [`T{}`] (prelude.carbon:{{\d+}})