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

@@ -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) {

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<std::pair<SemIR::ConstantId, SemIR::SpecificInterfaceId>,
+          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<ImplLookupStackEntry> 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.

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<SemIR::FunctionType>(struct_value.type_id),

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

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);
         }
       }

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

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: <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> = 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: <witness> = 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> = 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: <witness> = impl_witness %impl_witness_table [concrete = constants.%impl_witness]
+// CHECK:STDOUT:   <elided>
+// 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> = specific_function %Copy.ref, @Copy.loc6(constants.%Copy.facet.26f) [concrete = constants.%Copy.specific_fn]
+// CHECK:STDOUT:   <elided>
+// 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> = bound_method %p.ref, %impl.elem0
+// CHECK:STDOUT:   %specific_fn: <specific function> = 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> = 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: