Key context improvements (#4095)

This injects a customization point for hashtable-specific equality
testing that the key context uses by default. While this is rarely
needed, there are LLVM types where it is necessary and it seems a good
general tool to have to avoid unnecessary complexity from custom key
contexts when a simple customization of equality is all that is
required.

This also adds a CRTP mixin for implementing a common pattern of key
contexts where the context provides translation of some key types into
another type, potentially using state. Rather than having to implement
the entire key context API, code can derive from this template and
simply provide a set of overloads for the types it wants to translate.
Any key types used which can be passed to one of those overloads will
get translated before following the same logic as the default key
context. While this updates the only usage so far of this pattern, a
subsequent PR will add several more users making the pattern worth
abstracting here.

.codespell_ignore

@@ -15,4 +15,5 @@ groupt
 inout
 parameteras
 pullrequest
+rightt
 statics

common/BUILD

@@ -188,6 +188,18 @@ cc_library(
     hdrs = ["hashtable_key_context.h"],
     deps = [
         ":hashing",
+        "@llvm-project//llvm:Support",
+    ],
+)
+
+cc_test(
+    name = "hashtable_key_context_test",
+    srcs = ["hashtable_key_context_test.cpp"],
+    deps = [
+        ":hashtable_key_context",
+        "//testing/base:gtest_main",
+        "@googletest//:gtest",
+        "@llvm-project//llvm:Support",
     ],
 )
 

common/hashtable_key_context.h

@@ -5,10 +5,38 @@
 #ifndef CARBON_COMMON_HASHTABLE_KEY_CONTEXT_H_
 #define CARBON_COMMON_HASHTABLE_KEY_CONTEXT_H_
 
+#include <concepts>
+
 #include "common/hashing.h"
+#include "llvm/ADT/APFloat.h"
+#include "llvm/ADT/APInt.h"
 
 namespace Carbon {
 
+// The equality comparison used by the the hashtable key contexts in this file,
+// and suitable for using in other hashtable key contexts.
+//
+// This provides a hashtable-specific extension point to implement equality
+// comparison within a hashtable key context. By default, it will use
+// `operator==` on the LHS and RHS operands. However, types can provide a
+// dedicated customization point by implementing a free function that can be
+// found by ADL for your type called `CarbonHashtableEq` with the following
+// signature:
+//
+// ```cpp
+// auto CarbonHashtableEq(const YourType& lhs, const YourType& rhs) -> bool;
+// ```
+//
+// Any such overload will be able to override the default we provide for types
+// that can compare with `==`.
+//
+// This library also provides any customization points for LLVM or standard
+// library types either lacking `operator==` or where that operator is not
+// suitable for hashtables. For example, `llvm::APInt` and `llvm::APFloat` have
+// custom equality comparisons provided through this extension point.
+template <typename LeftT, typename RightT>
+auto HashtableEq(const LeftT& lhs, const RightT& rhs) -> bool;
+
 // Customizable context for keys in hashtables.
 //
 // This type or customizations matching its API are used with the data
@@ -69,16 +97,138 @@ namespace Carbon {
 // };
 // ```
 struct DefaultKeyContext {
-  template <typename KeyT>
-  auto HashKey(const KeyT& key, uint64_t seed) const -> HashCode {
+  template <typename AnyKeyT>
+  auto HashKey(const AnyKeyT& key, uint64_t seed) const -> HashCode;
+
+  template <typename AnyKeyT, typename TableKeyT>
+  auto KeyEq(const AnyKeyT& lhs_key, const TableKeyT& rhs_key) const -> bool;
+};
+
+// A CRTP mixin for a custom key context type that first translates keys to a
+// different type, possibly using some state.
+//
+// Derived types should publicly inherit from this mixin and define overloads of
+// the `TranslateKey` method as indicated below in its comment.
+template <typename DerivedT>
+class TranslatingKeyContext {
+ public:
+  // Derived types should provide one or more overloads that hide this function
+  // and perform translation for the key types which need it.
+  //
+  // For any key type, the context will check if there exists a callable
+  // `TranslateKey` function on the derived type. If so, that function will be
+  // called and the result used for hashing or comparison. If not, the key will
+  // be used directly. The derived type doesn't need to and shouldn't provide a
+  // default no-op overload. Instead, for any types that need no translation, it
+  // should ensure no overload is viable.
+  //
+  // Note that this function should be *hidden* by the derived overloads. It is
+  // provided here to help detect typos or misspellings or cases where no
+  // overload is provided at all.
+  template <typename TranslateKeyT>
+  auto TranslateKey(const TranslateKeyT& /*key*/) const -> int {
+    // A static_assert that will fail on any actual instantiation (it can't be
+    // instantiated with a void type). We have to make this dependent as
+    // Clang-16 will fail to compile even when the definition is never
+    // instantiated otherwise.
+    static_assert(
+        std::same_as<TranslateKeyT, void>,
+        "No `TranslateKey` overload was provided by the derived type!");
+  }
+
+  template <typename AnyKeyT>
+  auto HashKey(const AnyKeyT& key, uint64_t seed) const -> HashCode;
+
+  template <typename AnyKeyT, typename TableKeyT>
+  auto KeyEq(const AnyKeyT& lhs_key, const TableKeyT& rhs_key) const -> bool;
+};
+
+////////////////////////////////////////////////////////////////////////////////
+//
+// Only implementation details below this point.
+//
+////////////////////////////////////////////////////////////////////////////////
+
+namespace InternalHashtableEqDispatch {
+
+inline auto CarbonHashtableEq(const llvm::APInt& lhs, const llvm::APInt& rhs)
+    -> bool {
+  return lhs.getBitWidth() == rhs.getBitWidth() && lhs == rhs;
+}
+
+inline auto CarbonHashtableEq(const llvm::APFloat& lhs,
+                              const llvm::APFloat& rhs) -> bool {
+  return lhs.bitwiseIsEqual(rhs);
+}
+
+template <typename LeftT, typename RightT>
+inline auto CarbonHashtableEq(const LeftT& lhs, const RightT& rhs) -> bool
+  requires(requires {
+    { lhs == rhs } -> std::convertible_to<bool>;
+  })
+{
+  return lhs == rhs;
+}
+
+template <typename LeftT, typename RightT>
+inline auto DispatchImpl(const LeftT& lhs, const RightT& rhs) -> bool {
+  // This unqualified call will find both the overloads in our internal
+  // namespace above and ADL-found functions within an associated namespace for
+  // either `LeftT` or `RightT`.
+  return CarbonHashtableEq(lhs, rhs);
+}
+
+}  // namespace InternalHashtableEqDispatch
+
+template <typename LeftT, typename RightT>
+inline auto HashtableEq(const LeftT& lhs, const RightT& rhs) -> bool {
+  return InternalHashtableEqDispatch::DispatchImpl(lhs, rhs);
+}
+
+template <typename AnyKeyT>
+auto DefaultKeyContext::HashKey(const AnyKeyT& key, uint64_t seed) const
+    -> HashCode {
+  return HashValue(key, seed);
+}
+
+template <typename AnyKeyT, typename TableKeyT>
+auto DefaultKeyContext::KeyEq(const AnyKeyT& lhs_key,
+                              const TableKeyT& rhs_key) const -> bool {
+  return HashtableEq(lhs_key, rhs_key);
+}
+
+template <typename DerivedT>
+template <typename AnyKeyT>
+auto TranslatingKeyContext<DerivedT>::HashKey(const AnyKeyT& key,
+                                              uint64_t seed) const -> HashCode {
+  const DerivedT& self = *static_cast<const DerivedT*>(this);
+  if constexpr (requires { self.TranslateKey(key); }) {
+    return HashValue(self.TranslateKey(key), seed);
+  } else {
     return HashValue(key, seed);
   }
+}
 
-  template <typename LHSKeyT, typename RHSKeyT>
-  auto KeyEq(const LHSKeyT& lhs_key, const RHSKeyT& rhs_key) const -> bool {
-    return lhs_key == rhs_key;
+template <typename DerivedT>
+template <typename AnyKeyT, typename TableKeyT>
+auto TranslatingKeyContext<DerivedT>::KeyEq(const AnyKeyT& lhs_key,
+                                            const TableKeyT& rhs_key) const
+    -> bool {
+  const DerivedT& self = *static_cast<const DerivedT*>(this);
+  // Because we don't want to make no-op calls and potentially struggle with
+  // temporary lifetimes at runtime we have to fully expand the 4 states.
+  constexpr bool TranslateLHS = requires { self.TranslateKey(lhs_key); };
+  constexpr bool TranslateRHS = requires { self.TranslateKey(rhs_key); };
+  if constexpr (TranslateLHS && TranslateRHS) {
+    return HashtableEq(self.TranslateKey(lhs_key), self.TranslateKey(rhs_key));
+  } else if constexpr (TranslateLHS) {
+    return HashtableEq(self.TranslateKey(lhs_key), rhs_key);
+  } else if constexpr (TranslateRHS) {
+    return HashtableEq(lhs_key, self.TranslateKey(rhs_key));
+  } else {
+    return HashtableEq(lhs_key, rhs_key);
   }
-};
+}
 
 }  // namespace Carbon
 

common/hashtable_key_context_test.cpp

@@ -0,0 +1,219 @@
+// 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
+
+#include "common/hashtable_key_context.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+namespace Carbon {
+namespace {
+
+using ::testing::Eq;
+using ::testing::Ne;
+
+struct DefaultEq {
+  int x, y;
+
+  friend auto operator==(const DefaultEq& lhs, const DefaultEq& rhs)
+      -> bool = default;
+};
+
+struct CustomEq {
+  int x, y;
+
+  friend auto operator==(const CustomEq& lhs, const CustomEq& rhs) -> bool {
+    return lhs.x == rhs.x && lhs.y == rhs.y;
+  }
+};
+
+struct CustomExtEq {
+  int x, y;
+
+  friend auto CarbonHashtableEq(const CustomExtEq& lhs, const CustomExtEq& rhs)
+      -> bool {
+    return lhs.x == rhs.x && lhs.y == rhs.y;
+  }
+};
+
+TEST(HashtableKeyContextTest, HashtableEq) {
+  EXPECT_TRUE(HashtableEq(0, 0));
+  EXPECT_FALSE(HashtableEq(1, 0));
+  EXPECT_FALSE(HashtableEq(0, 1));
+  EXPECT_FALSE(HashtableEq(1234, 5678));
+  EXPECT_TRUE(HashtableEq(5678, 5678));
+
+  EXPECT_TRUE(
+      HashtableEq(DefaultEq{.x = 0, .y = 0}, DefaultEq{.x = 0, .y = 0}));
+  EXPECT_FALSE(
+      HashtableEq(DefaultEq{.x = 1, .y = 2}, DefaultEq{.x = 3, .y = 4}));
+
+  EXPECT_TRUE(HashtableEq(CustomEq{.x = 0, .y = 0}, CustomEq{.x = 0, .y = 0}));
+  EXPECT_FALSE(HashtableEq(CustomEq{.x = 1, .y = 2}, CustomEq{.x = 3, .y = 4}));
+
+  EXPECT_TRUE(
+      HashtableEq(CustomExtEq{.x = 0, .y = 0}, CustomExtEq{.x = 0, .y = 0}));
+  EXPECT_FALSE(
+      HashtableEq(CustomExtEq{.x = 1, .y = 2}, CustomExtEq{.x = 3, .y = 4}));
+}
+
+TEST(HashtableKeyContextTest, HashtableEqAPInt) {
+  // Hashtable equality doesn't assert on mismatched bit width, it includes the
+  // bit width in the comparison.
+  llvm::APInt one_64(/*numBits=*/64, /*val=*/1);
+  llvm::APInt two_64(/*numBits=*/64, /*val=*/2);
+  llvm::APInt one_128(/*numBits=*/128, /*val=*/1);
+  llvm::APInt two_128(/*numBits=*/128, /*val=*/2);
+
+  EXPECT_TRUE(HashtableEq(one_64, one_64));
+  EXPECT_FALSE(HashtableEq(one_64, one_128));
+  EXPECT_TRUE(HashtableEq(two_128, two_128));
+  EXPECT_FALSE(HashtableEq(two_64, two_128));
+  EXPECT_FALSE(HashtableEq(one_64, two_64));
+  EXPECT_FALSE(HashtableEq(one_64, two_128));
+  EXPECT_FALSE(HashtableEq(one_128, two_128));
+  EXPECT_FALSE(HashtableEq(one_128, two_64));
+}
+
+TEST(HashtableKeyContextTest, HashtableEqAPFloat) {
+  // Hashtable equality for `APFloat` uses a bitwise comparison. This
+  // differentiates between various things that would otherwise not make sense:
+  // - Different floating point semantics
+  // - `-0.0` and `0.0`
+  //
+  // It also allows NaNs to be compared meaningfully.
+  llvm::APFloat zero_float =
+      llvm::APFloat::getZero(llvm::APFloat::IEEEsingle());
+  llvm::APFloat neg_zero_float =
+      llvm::APFloat::getZero(llvm::APFloat::IEEEsingle(), /*Negative=*/true);
+  llvm::APFloat zero_double =
+      llvm::APFloat::getZero(llvm::APFloat::IEEEdouble());
+  llvm::APFloat zero_bfloat = llvm::APFloat::getZero(llvm::APFloat::BFloat());
+  llvm::APFloat one_float = llvm::APFloat::getOne(llvm::APFloat::IEEEsingle());
+  llvm::APFloat inf_float = llvm::APFloat::getInf(llvm::APFloat::IEEEsingle());
+  llvm::APFloat nan_0_float = llvm::APFloat::getNaN(
+      llvm::APFloat::IEEEsingle(), /*Negative=*/false, /*payload=*/0);
+  llvm::APFloat nan_42_float = llvm::APFloat::getNaN(
+      llvm::APFloat::IEEEsingle(), /*Negative=*/false, /*payload=*/42);
+
+  // Boring cases.
+  EXPECT_TRUE(HashtableEq(zero_float, zero_float));
+  EXPECT_FALSE(HashtableEq(zero_float, one_float));
+  EXPECT_TRUE(HashtableEq(inf_float, inf_float));
+  EXPECT_FALSE(HashtableEq(inf_float, one_float));
+
+  // Confirm a case where we expect `==` to work but produce a different result.
+  ASSERT_TRUE(zero_float == neg_zero_float);
+  EXPECT_FALSE(HashtableEq(zero_float, neg_zero_float));
+
+  // Now work through less reasonable things outside of a hashtable such as
+  // mixing semantics and NaNs.
+  EXPECT_FALSE(HashtableEq(zero_float, zero_double));
+  EXPECT_FALSE(HashtableEq(zero_float, zero_bfloat));
+  EXPECT_FALSE(HashtableEq(zero_float, nan_0_float));
+  EXPECT_FALSE(HashtableEq(zero_float, nan_42_float));
+  EXPECT_FALSE(HashtableEq(nan_0_float, nan_42_float));
+}
+
+struct CustomHash {
+  int x;
+
+  friend auto CarbonHashValue(const CustomHash& value, uint64_t seed)
+      -> HashCode {
+    return HashValue(value.x + 42, seed);
+  }
+};
+
+TEST(HashtableKeyContextTest, DefaultKeyContext) {
+  // Make sure the default context dispatches appropriately, including for
+  // interesting types. We don't cover all the cases here and use the direct
+  // tests of `HashtableEq` for that.
+  DefaultKeyContext context;
+
+  EXPECT_FALSE(context.KeyEq(1234, 5678));
+  EXPECT_TRUE(context.KeyEq(5678, 5678));
+  EXPECT_TRUE(context.KeyEq(DefaultEq{0, 0}, DefaultEq{0, 0}));
+  EXPECT_FALSE(context.KeyEq(DefaultEq{1, 2}, DefaultEq{3, 4}));
+  EXPECT_TRUE(context.KeyEq(CustomEq{0, 0}, CustomEq{0, 0}));
+  EXPECT_FALSE(context.KeyEq(CustomEq{1, 2}, CustomEq{3, 4}));
+  EXPECT_TRUE(context.KeyEq(CustomExtEq{0, 0}, CustomExtEq{0, 0}));
+  EXPECT_FALSE(context.KeyEq(CustomExtEq{1, 2}, CustomExtEq{3, 4}));
+
+  llvm::APInt one_64(/*numBits=*/64, /*val=*/1);
+  llvm::APInt one_128(/*numBits=*/128, /*val=*/1);
+  EXPECT_TRUE(HashtableEq(one_64, one_64));
+  EXPECT_FALSE(HashtableEq(one_64, one_128));
+
+  llvm::APFloat zero_float =
+      llvm::APFloat::getZero(llvm::APFloat::IEEEsingle());
+  llvm::APFloat neg_zero_float =
+      llvm::APFloat::getZero(llvm::APFloat::IEEEsingle(), /*Negative=*/true);
+  EXPECT_TRUE(HashtableEq(zero_float, zero_float));
+  EXPECT_FALSE(HashtableEq(zero_float, neg_zero_float));
+
+  // Also check hash dispatching.
+  uint64_t seed = 1234;
+  EXPECT_THAT(context.HashKey(42, seed), Eq(HashValue(42, seed)));
+  EXPECT_THAT(context.HashKey(CustomHash{.x = 1234}, seed),
+              Eq(HashValue(CustomHash{.x = 1234}, seed)));
+  EXPECT_THAT(context.HashKey(one_64, seed), Eq(HashValue(one_64, seed)));
+  EXPECT_THAT(context.HashKey(one_128, seed), Eq(HashValue(one_128, seed)));
+  EXPECT_THAT(context.HashKey(one_64, seed),
+              Ne(context.HashKey(one_128, seed)));
+  EXPECT_THAT(context.HashKey(zero_float, seed),
+              Eq(HashValue(zero_float, seed)));
+  EXPECT_THAT(context.HashKey(neg_zero_float, seed),
+              Eq(HashValue(neg_zero_float, seed)));
+  EXPECT_THAT(context.HashKey(zero_float, seed),
+              Ne(context.HashKey(neg_zero_float, seed)));
+}
+
+struct TestTranslatingKeyContext
+    : TranslatingKeyContext<TestTranslatingKeyContext> {
+  auto TranslateKey(int index) const -> const llvm::APInt& {
+    return array[index];
+  }
+
+  llvm::ArrayRef<llvm::APInt> array;
+};
+
+TEST(HashtableKeyContextTest, TranslatingKeyContext) {
+  llvm::APInt one_64(/*numBits=*/64, /*val=*/1);
+  llvm::APInt two_64(/*numBits=*/64, /*val=*/2);
+  llvm::APInt one_128(/*numBits=*/128, /*val=*/1);
+  llvm::APInt two_128(/*numBits=*/128, /*val=*/2);
+  // An array of values, including some duplicates.
+  llvm::SmallVector<llvm::APInt> values = {one_64,  two_64, one_128,
+                                           two_128, one_64, one_64};
+
+  TestTranslatingKeyContext context = {.array = values};
+
+  uint64_t seed = 1234;
+  EXPECT_THAT(context.HashKey(0, seed), Eq(HashValue(one_64, seed)));
+  EXPECT_THAT(context.HashKey(1, seed), Eq(HashValue(two_64, seed)));
+  EXPECT_THAT(context.HashKey(2, seed), Eq(HashValue(one_128, seed)));
+  EXPECT_THAT(context.HashKey(3, seed), Eq(HashValue(two_128, seed)));
+  EXPECT_THAT(context.HashKey(4, seed), Eq(HashValue(one_64, seed)));
+  EXPECT_THAT(context.HashKey(5, seed), Eq(HashValue(one_64, seed)));
+
+  EXPECT_TRUE(context.KeyEq(one_64, 0));
+  EXPECT_TRUE(context.KeyEq(one_64, 4));
+  EXPECT_TRUE(context.KeyEq(one_64, 5));
+  EXPECT_TRUE(context.KeyEq(0, one_64));
+  EXPECT_TRUE(context.KeyEq(0, 0));
+  EXPECT_TRUE(context.KeyEq(0, 4));
+  EXPECT_TRUE(context.KeyEq(4, 5));
+  EXPECT_FALSE(context.KeyEq(one_64, 1));
+  EXPECT_FALSE(context.KeyEq(one_64, 2));
+  EXPECT_FALSE(context.KeyEq(one_64, 3));
+  EXPECT_FALSE(context.KeyEq(1, one_64));
+  EXPECT_FALSE(context.KeyEq(2, one_64));
+  EXPECT_FALSE(context.KeyEq(3, one_64));
+  EXPECT_FALSE(context.KeyEq(0, 1));
+  EXPECT_FALSE(context.KeyEq(0, 2));
+  EXPECT_FALSE(context.KeyEq(4, 3));
+}
+
+}  // namespace
+}  // namespace Carbon

common/map.h

@@ -224,7 +224,7 @@ class MapBase : protected RawHashtable::BaseImpl<InputKeyT, InputValueT,
 
   // Convenience forwarder to the view type.
   template <typename CallbackT>
-  void ForEach(CallbackT callback)
+  void ForEach(CallbackT callback) const
     requires(std::invocable<CallbackT, KeyT&, ValueT&>)
   {
     return ViewT(*this).ForEach(callback);

common/raw_hashtable.h

@@ -16,7 +16,6 @@
 
 #include "common/check.h"
 #include "common/hashing.h"
-#include "common/hashtable_key_context.h"
 #include "common/raw_hashtable_metadata_group.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/MathExtras.h"
@@ -301,8 +300,7 @@ struct Metrics {
 struct Storage {};
 
 // Forward declaration to support friending, see the definition below.
-template <typename KeyT, typename ValueT = void,
-          typename InputKeyContextT = DefaultKeyContext>
+template <typename InputKeyT, typename InputValueT, typename InputKeyContextT>
 class BaseImpl;
 
 // Implementation helper for defining a read-only view type for a hashtable.
@@ -326,8 +324,7 @@ class BaseImpl;
 // Some methods are used by other parts of the raw hashtable implementation.
 // Those are kept `private` and where necessary the other components of the raw
 // hashtable implementation are friended to give access to them.
-template <typename InputKeyT, typename InputValueT = void,
-          typename InputKeyContextT = DefaultKeyContext>
+template <typename InputKeyT, typename InputValueT, typename InputKeyContextT>
 class ViewImpl {
  protected:
   using KeyT = InputKeyT;

common/raw_hashtable_test_helpers.h

@@ -86,20 +86,17 @@ struct FixedHashKeyContext : DefaultKeyContext {
 };
 
 template <typename T>
-class IndexKeyContext {
+class IndexKeyContext : public TranslatingKeyContext<IndexKeyContext<T>> {
+  using Base = TranslatingKeyContext<IndexKeyContext>;
+
  public:
   explicit IndexKeyContext(llvm::ArrayRef<T> array) : array_(array) {}
 
-  auto HashKey(const T& value, uint64_t seed) const -> HashCode {
-    return HashValue(value, seed);
-  }
-  auto HashKey(ssize_t index, uint64_t seed) const -> HashCode {
-    return HashKey(array_[index], seed);
-  }
+  auto TranslateKey(ssize_t index) const -> const T& { return array_[index]; }
 
-  auto KeyEq(const T& lhs, ssize_t rhs_index) const -> bool {
-    return lhs == array_[rhs_index];
-  }
+  // Override the CRTP approach when we have two indices as we can optimize that
+  // approach.
+  using Base::KeyEq;
   auto KeyEq(ssize_t lhs_index, ssize_t rhs_index) const -> bool {
     // No need to compare the elements, if the indices are equal, the values
     // must be.

toolchain/sem_ir/generic.cpp

@@ -6,7 +6,8 @@
 
 namespace Carbon::SemIR {
 
-class GenericInstanceStore::KeyContext {
+class GenericInstanceStore::KeyContext
+    : public TranslatingKeyContext<KeyContext> {
  public:
   // A lookup key for a generic instance.
   struct Key {
@@ -19,21 +20,10 @@ class GenericInstanceStore::KeyContext {
   explicit KeyContext(llvm::ArrayRef<GenericInstance> instances)
       : instances_(instances) {}
 
-  auto AsKey(GenericInstanceId id) const -> Key {
+  auto TranslateKey(GenericInstanceId id) const -> Key {
     const auto& instance = instances_[id.index];
     return {.generic_id = instance.generic_id, .args_id = instance.args_id};
   }
-  static auto AsKey(Key key) -> Key { return key; }
-
-  template <typename KeyT>
-  auto HashKey(KeyT key, uint64_t seed) const -> HashCode {
-    return HashValue(AsKey(key), seed);
-  }
-
-  template <typename LHSKeyT, typename RHSKeyT>
-  auto KeyEq(const LHSKeyT& lhs_key, const RHSKeyT& rhs_key) const -> bool {
-    return AsKey(lhs_key) == AsKey(rhs_key);
-  }
 
  private:
   llvm::ArrayRef<GenericInstance> instances_;