From c77eebd15e19adb9f20f81fc7c9f383b77c50806 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 2 Dec 2025 14:51:25 -0800 Subject: [PATCH] Cache final impl lookup results. (#6452) If an impl lookup finds a final result, cache that and reuse it if we perform the same lookup later. In addition to reducing repeated work, this allows us to produce the same result for repeated lookups that find a C++ operator. This isn't a great solution to that problem, as it's not clear how to extend it to behave correctly across import, but we don't have a solution for that for C++ interop in general. --- toolchain/check/check_unit.cpp | 2 +- toolchain/check/context.h | 10 +++ toolchain/check/cpp/impl_lookup.cpp | 3 + toolchain/check/eval_inst.cpp | 2 +- toolchain/check/impl_lookup.cpp | 28 ++++++- toolchain/check/impl_lookup.h | 18 +++- .../testdata/interop/cpp/impls/copy.carbon | 82 ++++++++++++++----- 7 files changed, 116 insertions(+), 29 deletions(-) diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp index 5a9b4c110a97..e7c04146677a 100644 --- a/toolchain/check/check_unit.cpp +++ b/toolchain/check/check_unit.cpp @@ -526,7 +526,7 @@ auto CheckUnit::CheckPoisonedConcreteImplLookupQueries() -> void { for (const auto& poison : poisoned_queries) { auto witness_result = EvalLookupSingleImplWitness( context_, poison.loc_id, poison.query, poison.query.query_self_inst_id, - /*poison_final_results=*/false); + EvalImplLookupMode::RecheckPoisonedLookup); CARBON_CHECK(witness_result.has_final_value()); auto found_witness_id = witness_result.final_witness(); if (found_witness_id == SemIR::ErrorInst::InstId) { diff --git a/toolchain/check/context.h b/toolchain/check/context.h index c69b30ef04e4..df8c90b604a4 100644 --- a/toolchain/check/context.h +++ b/toolchain/check/context.h @@ -227,6 +227,12 @@ class Context { return impl_lookup_stack_; } + // A map from a (self, interface) pair to a final witness. + using ImplLookupCacheMap = + Map, + SemIR::InstId>; + auto impl_lookup_cache() -> ImplLookupCacheMap& { return impl_lookup_cache_; } + // An impl lookup query that resulted in a concrete witness from finding an // `impl` declaration (not though a facet value), and its result. Used to look // for conflicting `impl` declarations. @@ -474,6 +480,10 @@ class Context { // via the acyclic rule and the termination rule. llvm::SmallVector impl_lookup_stack_; + // Tracks a mapping from (self, interface) to witness, for queries that had + // final results. + ImplLookupCacheMap impl_lookup_cache_; + // Tracks impl lookup queries that lead to concrete witness results, along // with those results. Used to verify that the same queries produce the same // results at the end of the file. Any difference is diagnosed. diff --git a/toolchain/check/cpp/impl_lookup.cpp b/toolchain/check/cpp/impl_lookup.cpp index 29361d1c16fc..60ce0ca9b4d2 100644 --- a/toolchain/check/cpp/impl_lookup.cpp +++ b/toolchain/check/cpp/impl_lookup.cpp @@ -101,6 +101,9 @@ static auto BuildWitness(Context& context, SemIR::LocId loc_id, if (struct_value.type_id == SemIR::ErrorInst::TypeId) { return SemIR::ErrorInst::InstId; } + // TODO: If a thunk is needed, this will build a different value each + // time it's called, so we won't properly deduplicate repeated + // witnesses. witness_value_id = CheckAssociatedFunctionImplementation( context, context.types().GetAs(struct_value.type_id), diff --git a/toolchain/check/eval_inst.cpp b/toolchain/check/eval_inst.cpp index b227d8a98220..8c58d1f0feee 100644 --- a/toolchain/check/eval_inst.cpp +++ b/toolchain/check/eval_inst.cpp @@ -298,7 +298,7 @@ auto EvalConstantInst(Context& context, SemIR::InstId inst_id, auto result = EvalLookupSingleImplWitness(context, SemIR::LocId(inst_id), inst, self_facet_value_inst_id, - /*poison_final_results=*/true); + EvalImplLookupMode::Normal); if (!result.has_value()) { // We use NotConstant to communicate back to impl lookup that the lookup // failed. This can not happen for a deferred symbolic lookup in a generic diff --git a/toolchain/check/impl_lookup.cpp b/toolchain/check/impl_lookup.cpp index 9d15407001a0..f60099ef458b 100644 --- a/toolchain/check/impl_lookup.cpp +++ b/toolchain/check/impl_lookup.cpp @@ -921,7 +921,7 @@ static auto GetFacetAsType(Context& context, SemIR::LocId loc_id, auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id, SemIR::LookupImplWitness eval_query, SemIR::InstId self_facet_value_inst_id, - bool poison_final_results) + EvalImplLookupMode mode) -> EvalImplLookupResult { auto query_specific_interface = context.specific_interfaces().Get(eval_query.query_specific_interface_id); @@ -938,6 +938,17 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id, SemIR::ConstantId query_self_const_id = context.constant_values().Get(eval_query.query_self_inst_id); + // Check to see if this result is in the cache. But skip the cache if + // //we're re-checking a poisoned result and need to redo the lookup. + std::pair impl_lookup_cache_key = {query_self_const_id, + eval_query.query_specific_interface_id}; + if (mode != EvalImplLookupMode::RecheckPoisonedLookup) { + if (auto result = + context.impl_lookup_cache().Lookup(impl_lookup_cache_key)) { + return EvalImplLookupResult::MakeFinal(result.value()); + } + } + // The kind of lookup we're performing, which determines what kind of result // we provide. enum LookupKind { @@ -1023,7 +1034,8 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id, // If the impl was effectively final, then we don't need to poison here. A // change of query result will already be diagnosed at the point where the // new impl decl was written that changes the result. - if (poison_final_results && result.has_final_value() && + if (mode != EvalImplLookupMode::RecheckPoisonedLookup && + result.has_final_value() && !IsImplEffectivelyFinal(context, context.impls().Get(candidate.impl_id))) { context.poisoned_concrete_impl_lookup_queries().push_back( @@ -1042,10 +1054,16 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id, SemIR::LocId( context.impls().Get(candidate.impl_id).first_owning_decl_id)); if (cpp_witness_id.has_value()) { - return EvalImplLookupResult::MakeFinal(cpp_witness_id); + result = EvalImplLookupResult::MakeFinal(cpp_witness_id); } } + if (mode != EvalImplLookupMode::RecheckPoisonedLookup && + result.has_final_value()) { + context.impl_lookup_cache().Insert(impl_lookup_cache_key, + result.final_witness()); + } + return result; } } @@ -1061,6 +1079,10 @@ auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id, GetFacetAsType(context, loc_id, query_self_const_id), query_specific_interface, nullptr, SemIR::LocId::None); if (cpp_witness_id.has_value()) { + if (mode != EvalImplLookupMode::RecheckPoisonedLookup) { + context.impl_lookup_cache().Insert(impl_lookup_cache_key, + cpp_witness_id); + } return EvalImplLookupResult::MakeFinal(cpp_witness_id); } } diff --git a/toolchain/check/impl_lookup.h b/toolchain/check/impl_lookup.h index d86e10b3583f..83ef2258143b 100644 --- a/toolchain/check/impl_lookup.h +++ b/toolchain/check/impl_lookup.h @@ -96,18 +96,28 @@ class [[nodiscard]] EvalImplLookupResult { Value value_; }; +// The kind of impl lookup being performed by a call to +// `EvalLookupSingleImplWitness`. +enum class EvalImplLookupMode { + // This is a regular impl lookup performed during check. If we produce a final + // witness value that uses a specializable impl, the query will be poisoned so + // that we will recheck it at the end of the compilation. + Normal, + // This is a re-check of a poisoned lookup being performed at the end of a + // file. This disables any caching of lookup results for this query and redoes + // the impl lookup. + RecheckPoisonedLookup, +}; + // Looks for a witness instruction of an impl declaration for a query consisting // of a type value or facet value, and a single interface. This is for eval to // execute lookup via the LookupImplWitness instruction. It does not consider // the self facet value for finding a witness, since LookupImplWitness() would // have found that and not caused us to defer lookup to here. -// -// `poison_final_results` poisons lookup results which are effectively final, -// preventing overlapping final impls. auto EvalLookupSingleImplWitness(Context& context, SemIR::LocId loc_id, SemIR::LookupImplWitness eval_query, SemIR::InstId self_facet_value_inst_id, - bool poison_final_results) + EvalImplLookupMode mode) -> EvalImplLookupResult; } // namespace Carbon::Check diff --git a/toolchain/check/testdata/interop/cpp/impls/copy.carbon b/toolchain/check/testdata/interop/cpp/impls/copy.carbon index cd713d948456..ea70bf95bff8 100644 --- a/toolchain/check/testdata/interop/cpp/impls/copy.carbon +++ b/toolchain/check/testdata/interop/cpp/impls/copy.carbon @@ -162,7 +162,7 @@ fn CopyProtectedCopy(c: Cpp.ProtectedCopy) -> Cpp.ProtectedCopy { return c; } -// --- fail_todo_copy_generically.carbon +// --- copy_generically.carbon library "[[@TEST_NAME]]"; @@ -173,32 +173,17 @@ fn Copy[T:! Core.Copy](c: T) -> T { } fn DoCopy(c: Cpp.Copyable) -> Cpp.Copyable { - // TODO: We perform multiple impl lookups for `Cpp.Copyable as Core.Copy` - // here, and each one produces a different impl witness, resulting in a - // deduction failure. - // CHECK:STDERR: fail_todo_copy_generically.carbon:[[@LINE+7]]:10: error: inconsistent deductions for value of generic parameter `T` [DeductionInconsistent] - // CHECK:STDERR: return Copy(c); - // CHECK:STDERR: ^~~~~~~ - // CHECK:STDERR: fail_todo_copy_generically.carbon:[[@LINE-11]]:1: note: while deducing parameters of generic declared here [DeductionGenericHere] - // CHECK:STDERR: fn Copy[T:! Core.Copy](c: T) -> T { - // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - // CHECK:STDERR: + //@dump-sem-ir-begin return Copy(c); + //@dump-sem-ir-end } class Wrap(T:! Core.Copy) {} -// TODO: This should type-check: each conversion of `Cpp.Copyable` to -// `Core.Copy` should produce the same value. fn EqualWitnesses(p: Wrap(Cpp.Copyable)*) -> Wrap(Cpp.Copyable)* { - // CHECK:STDERR: fail_todo_copy_generically.carbon:[[@LINE+7]]:3: error: cannot implicitly convert expression of type `Wrap(Cpp.Copyable as Core.Copy)*` to `Wrap(Cpp.Copyable as Core.Copy)*` [ConversionFailure] - // CHECK:STDERR: return p; - // CHECK:STDERR: ^~~~~~~~~ - // CHECK:STDERR: fail_todo_copy_generically.carbon:[[@LINE+4]]:3: note: type `Wrap(Cpp.Copyable as Core.Copy)*` does not implement interface `Core.ImplicitAs(Wrap(Cpp.Copyable as Core.Copy)*)` [MissingImplInMemberAccessNote] - // CHECK:STDERR: return p; - // CHECK:STDERR: ^~~~~~~~~ - // CHECK:STDERR: + //@dump-sem-ir-begin return p; + //@dump-sem-ir-end } // CHECK:STDOUT: --- copy_copyable.carbon @@ -300,3 +285,60 @@ fn EqualWitnesses(p: Wrap(Cpp.Copyable)*) -> Wrap(Cpp.Copyable)* { // CHECK:STDOUT: return %.loc14_10.5 to %return // CHECK:STDOUT: } // CHECK:STDOUT: +// CHECK:STDOUT: --- copy_generically.carbon +// CHECK:STDOUT: +// CHECK:STDOUT: constants { +// CHECK:STDOUT: %Copy.type.705: type = facet_type <@Copy.1> [concrete] +// CHECK:STDOUT: %Copy.type.6f0: type = fn_type @Copy.loc6 [concrete] +// CHECK:STDOUT: %Copy: %Copy.type.6f0 = struct_value () [concrete] +// CHECK:STDOUT: %Copy.Op.type: type = fn_type @Copy.Op [concrete] +// CHECK:STDOUT: %T.d9f: type = symbolic_binding T, 0 [symbolic] +// CHECK:STDOUT: %ptr.as.Copy.impl.Op.type.75b: type = fn_type @ptr.as.Copy.impl.Op, @ptr.as.Copy.impl(%T.d9f) [symbolic] +// CHECK:STDOUT: %ptr.as.Copy.impl.Op.692: %ptr.as.Copy.impl.Op.type.75b = struct_value () [symbolic] +// CHECK:STDOUT: %Copyable: type = class_type @Copyable [concrete] +// CHECK:STDOUT: %impl_witness: = impl_witness @DoCopy.%impl_witness_table [concrete] +// CHECK:STDOUT: %Copy.facet.26f: %Copy.type.705 = facet_value %Copyable, (%impl_witness) [concrete] +// CHECK:STDOUT: %Copy.specific_fn: = specific_function %Copy, @Copy.loc6(%Copy.facet.26f) [concrete] +// CHECK:STDOUT: %Wrap.248: type = class_type @Wrap, @Wrap(%Copy.facet.26f) [concrete] +// CHECK:STDOUT: %ptr.510: type = ptr_type %Wrap.248 [concrete] +// CHECK:STDOUT: %Copy.impl_witness.cab: = impl_witness imports.%Copy.impl_witness_table.67d, @ptr.as.Copy.impl(%Wrap.248) [concrete] +// CHECK:STDOUT: %ptr.as.Copy.impl.Op.type.b1b: type = fn_type @ptr.as.Copy.impl.Op, @ptr.as.Copy.impl(%Wrap.248) [concrete] +// CHECK:STDOUT: %ptr.as.Copy.impl.Op.ea7: %ptr.as.Copy.impl.Op.type.b1b = struct_value () [concrete] +// CHECK:STDOUT: %Copy.facet.5be: %Copy.type.705 = facet_value %ptr.510, (%Copy.impl_witness.cab) [concrete] +// CHECK:STDOUT: %.e13: type = fn_type_with_self_type %Copy.Op.type, %Copy.facet.5be [concrete] +// CHECK:STDOUT: %ptr.as.Copy.impl.Op.specific_fn: = specific_function %ptr.as.Copy.impl.Op.ea7, @ptr.as.Copy.impl.Op(%Wrap.248) [concrete] +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: imports { +// CHECK:STDOUT: %Core.import_ref.659: @ptr.as.Copy.impl.%ptr.as.Copy.impl.Op.type (%ptr.as.Copy.impl.Op.type.75b) = import_ref Core//prelude/parts/copy, loc{{\d+_\d+}}, loaded [symbolic = @ptr.as.Copy.impl.%ptr.as.Copy.impl.Op (constants.%ptr.as.Copy.impl.Op.692)] +// CHECK:STDOUT: %Copy.impl_witness_table.67d = impl_witness_table (%Core.import_ref.659), @ptr.as.Copy.impl [concrete] +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @DoCopy(%c.param: %Copyable) -> %return.param: %Copyable { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %Copy.ref: %Copy.type.6f0 = name_ref Copy, file.%Copy.decl [concrete = constants.%Copy] +// CHECK:STDOUT: %c.ref: %Copyable = name_ref c, %c +// CHECK:STDOUT: %impl_witness_table = impl_witness_table (%Copyable.Op.decl), invalid [concrete] +// CHECK:STDOUT: %impl_witness: = impl_witness %impl_witness_table [concrete = constants.%impl_witness] +// CHECK:STDOUT: +// CHECK:STDOUT: %Copy.facet.loc12_16.1: %Copy.type.705 = facet_value constants.%Copyable, (constants.%impl_witness) [concrete = constants.%Copy.facet.26f] +// CHECK:STDOUT: %.loc12_16.1: %Copy.type.705 = converted constants.%Copyable, %Copy.facet.loc12_16.1 [concrete = constants.%Copy.facet.26f] +// CHECK:STDOUT: %Copy.facet.loc12_16.2: %Copy.type.705 = facet_value constants.%Copyable, (constants.%impl_witness) [concrete = constants.%Copy.facet.26f] +// CHECK:STDOUT: %.loc12_16.2: %Copy.type.705 = converted constants.%Copyable, %Copy.facet.loc12_16.2 [concrete = constants.%Copy.facet.26f] +// CHECK:STDOUT: %Copy.specific_fn: = specific_function %Copy.ref, @Copy.loc6(constants.%Copy.facet.26f) [concrete = constants.%Copy.specific_fn] +// CHECK:STDOUT: +// CHECK:STDOUT: %Copy.call: init %Copyable = call %Copy.specific_fn(%c.ref) to %.loc10_28 +// CHECK:STDOUT: return %Copy.call to %return +// CHECK:STDOUT: } +// CHECK:STDOUT: +// CHECK:STDOUT: fn @EqualWitnesses(%p.param: %ptr.510) -> %ptr.510 { +// CHECK:STDOUT: !entry: +// CHECK:STDOUT: %p.ref: %ptr.510 = name_ref p, %p +// CHECK:STDOUT: %impl.elem0: %.e13 = impl_witness_access constants.%Copy.impl_witness.cab, element0 [concrete = constants.%ptr.as.Copy.impl.Op.ea7] +// CHECK:STDOUT: %bound_method.loc20_10.1: = bound_method %p.ref, %impl.elem0 +// CHECK:STDOUT: %specific_fn: = specific_function %impl.elem0, @ptr.as.Copy.impl.Op(constants.%Wrap.248) [concrete = constants.%ptr.as.Copy.impl.Op.specific_fn] +// CHECK:STDOUT: %bound_method.loc20_10.2: = bound_method %p.ref, %specific_fn +// CHECK:STDOUT: %ptr.as.Copy.impl.Op.call: init %ptr.510 = call %bound_method.loc20_10.2(%p.ref) +// CHECK:STDOUT: return %ptr.as.Copy.impl.Op.call to %return +// CHECK:STDOUT: } +// CHECK:STDOUT: