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: