Change CanonicalValueStore to take ValueT and KeyT as parameters (#5759)

`SpecificInterface` seems oddly placed. It appears to be in ids.h just
because it's used by typed_insts.h, but maybe that should be factored
differently? We typically aren't having typed_insts.h depend on non-ID
types. To that end, I'm splitting it out to its own file so that at
least I'm not adding a `ValueStore` dep inside ids.h

---------

Co-authored-by: Richard Smith <richard@metafoo.co.uk>

toolchain/base/BUILD

@@ -29,6 +29,7 @@ cc_test(
         ":value_ids",
         "//testing/base:gtest_main",
         "@googletest//:gtest",
+        "@llvm-project//llvm:Support",
     ],
 )
 
@@ -230,6 +231,7 @@ cc_library(
         ":value_ids",
         ":value_store",
         ":yaml",
+        "@llvm-project//llvm:Support",
     ],
 )
 

toolchain/base/canonical_value_store.h

@@ -17,18 +17,18 @@ namespace Carbon {
 // A wrapper for accumulating immutable values with deduplication, providing IDs
 // to later retrieve the value.
 //
-// `IdT::ValueType` must represent the type being indexed.
+// `ValueT` represents the type being stored.
 //
-// `IdT::KeyType` can optionally be present, and if so is used for the argument
-// to `Lookup`. It must be valid to use both `KeyType` and `ValueType` as lookup
-// types in the underlying `Set`.
-template <typename IdT>
+// `KeyT` can optionally be different from `ValueT`, and if so is used for the
+// argument to `Lookup`. It must be valid to use both `KeyT` and `ValueT` as
+// lookup types in the underlying `Set`.
+template <typename IdT, typename KeyT, typename ValueT = KeyT>
 class CanonicalValueStore {
  public:
-  using ValueType = ValueStoreTypes<IdT>::ValueType;
-  using KeyType = ValueStoreTypes<IdT>::KeyType;
-  using RefType = ValueStoreTypes<IdT>::RefType;
-  using ConstRefType = ValueStoreTypes<IdT>::ConstRefType;
+  using KeyType = std::remove_cvref_t<KeyT>;
+  using ValueType = ValueStoreTypes<IdT, ValueT>::ValueType;
+  using RefType = ValueStoreTypes<IdT, ValueT>::RefType;
+  using ConstRefType = ValueStoreTypes<IdT, ValueT>::ConstRefType;
 
   // Stores a canonical copy of the value and returns an ID to reference it.
   auto Add(ValueType value) -> IdT;
@@ -69,8 +69,8 @@ class CanonicalValueStore {
   Set<IdT, /*SmallSize=*/0, KeyContext> set_;
 };
 
-template <typename IdT>
-class CanonicalValueStore<IdT>::KeyContext
+template <typename IdT, typename KeyT, typename ValueT>
+class CanonicalValueStore<IdT, KeyT, ValueT>::KeyContext
     : public TranslatingKeyContext<KeyContext> {
  public:
   explicit KeyContext(const ValueStore<IdT, ValueType>* values)
@@ -84,22 +84,22 @@ class CanonicalValueStore<IdT>::KeyContext
   const ValueStore<IdT, ValueType>* values_;
 };
 
-template <typename IdT>
-auto CanonicalValueStore<IdT>::Add(ValueType value) -> IdT {
+template <typename IdT, typename KeyT, typename ValueT>
+auto CanonicalValueStore<IdT, KeyT, ValueT>::Add(ValueType value) -> IdT {
   auto make_key = [&] { return IdT(values_.Add(std::move(value))); };
   return set_.Insert(value, make_key, KeyContext(&values_)).key();
 }
 
-template <typename IdT>
-auto CanonicalValueStore<IdT>::Lookup(KeyType key) const -> IdT {
+template <typename IdT, typename KeyT, typename ValueT>
+auto CanonicalValueStore<IdT, KeyT, ValueT>::Lookup(KeyType key) const -> IdT {
   if (auto result = set_.Lookup(key, KeyContext(&values_))) {
     return result.key();
   }
   return IdT::None;
 }
 
-template <typename IdT>
-auto CanonicalValueStore<IdT>::Reserve(size_t size) -> void {
+template <typename IdT, typename KeyT, typename ValueT>
+auto CanonicalValueStore<IdT, KeyT, ValueT>::Reserve(size_t size) -> void {
   // Compute the resulting new insert count using the size of values -- the
   // set doesn't have a fast to compute current size.
   if (size > values_.size()) {

toolchain/base/canonical_value_store_test.cpp

@@ -9,6 +9,8 @@
 
 #include <string>
 
+#include "llvm/ADT/APFloat.h"
+#include "llvm/ADT/StringRef.h"
 #include "toolchain/base/value_ids.h"
 
 namespace Carbon::Testing {
@@ -21,7 +23,7 @@ TEST(CanonicalValueStore, Float) {
   llvm::APFloat float1(1.0);
   llvm::APFloat float2(2.0);
 
-  CanonicalValueStore<FloatId> floats;
+  CanonicalValueStore<FloatId, llvm::APFloat> floats;
   FloatId id1 = floats.Add(float1);
   FloatId id2 = floats.Add(float2);
 
@@ -36,7 +38,7 @@ TEST(CanonicalValueStore, Float) {
 TEST(CanonicalValueStore, Identifiers) {
   std::string a = "a";
   std::string b = "b";
-  CanonicalValueStore<IdentifierId> identifiers;
+  CanonicalValueStore<IdentifierId, llvm::StringRef> identifiers;
 
   // Make sure reserve works, we use it with identifiers.
   identifiers.Reserve(100);
@@ -58,7 +60,7 @@ TEST(CanonicalValueStore, Identifiers) {
 TEST(CanonicalValueStore, StringLiterals) {
   std::string a = "a";
   std::string b = "b";
-  CanonicalValueStore<StringLiteralValueId> string_literals;
+  CanonicalValueStore<StringLiteralValueId, llvm::StringRef> string_literals;
 
   auto a_id = string_literals.Add(a);
   auto b_id = string_literals.Add(b);

toolchain/base/int.h

@@ -54,7 +54,6 @@ struct IntStoreTestPeer;
 class IntId : public Printable<IntId> {
  public:
   static constexpr llvm::StringLiteral Label = "int";
-  using ValueType = llvm::APInt;
 
   // The encoding of integer IDs ensures that IDs associated with tokens during
   // lexing can fit into a compressed storage space. We arrange for
@@ -362,7 +361,6 @@ class IntStore {
   // Used for `values_`; tracked using `IntId`'s index range.
   struct APIntId : IdBase<APIntId> {
     static constexpr llvm::StringLiteral Label = "ap_int";
-    using ValueType = llvm::APInt;
     static const APIntId None;
     using IdBase::IdBase;
   };
@@ -422,7 +420,7 @@ class IntStore {
   auto LookupSignedLarge(llvm::APInt value) const -> IntId;
 
   // Stores values which don't fit in an IntId. These are always signed.
-  CanonicalValueStore<APIntId> values_;
+  CanonicalValueStore<APIntId, llvm::APInt> values_;
 };
 
 constexpr IntStore::APIntId IntStore::APIntId::None(IntId::None.AsIndex());

toolchain/base/shared_value_stores.h

@@ -5,6 +5,8 @@
 #ifndef CARBON_TOOLCHAIN_BASE_SHARED_VALUE_STORES_H_
 #define CARBON_TOOLCHAIN_BASE_SHARED_VALUE_STORES_H_
 
+#include "llvm/ADT/APFloat.h"
+#include "llvm/ADT/StringRef.h"
 #include "toolchain/base/canonical_value_store.h"
 #include "toolchain/base/int.h"
 #include "toolchain/base/mem_usage.h"
@@ -21,9 +23,10 @@ class SharedValueStores : public Yaml::Printable<SharedValueStores> {
   // Provide types that can be used by APIs to forward access to these stores.
   using IntStore = IntStore;
   using RealStore = ValueStore<RealId, Real>;
-  using FloatStore = CanonicalValueStore<FloatId>;
-  using IdentifierStore = CanonicalValueStore<IdentifierId>;
-  using StringLiteralStore = CanonicalValueStore<StringLiteralValueId>;
+  using FloatStore = CanonicalValueStore<FloatId, llvm::APFloat>;
+  using IdentifierStore = CanonicalValueStore<IdentifierId, llvm::StringRef>;
+  using StringLiteralStore =
+      CanonicalValueStore<StringLiteralValueId, llvm::StringRef>;
 
   explicit SharedValueStores() = default;
 

toolchain/base/value_ids.h

@@ -7,7 +7,6 @@
 
 #include "common/check.h"
 #include "common/ostream.h"
-#include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/YAMLParser.h"
@@ -45,7 +44,6 @@ struct Real : public Printable<Real> {
 // floating-point values in SemIR.
 struct FloatId : public IdBase<FloatId> {
   static constexpr llvm::StringLiteral Label = "float";
-  using ValueType = llvm::APFloat;
   static const FloatId None;
   using IdBase::IdBase;
 };
@@ -65,7 +63,6 @@ constexpr RealId RealId::None(RealId::NoneIndex);
 // non-negative.
 struct IdentifierId : public IdBase<IdentifierId> {
   static constexpr llvm::StringLiteral Label = "identifier";
-  using ValueType = llvm::StringRef;
   static const IdentifierId None;
   using IdBase::IdBase;
 };
@@ -111,7 +108,6 @@ constexpr PackageNameId PackageNameId::Core(PackageNameId::NoneIndex - 1);
 // Corresponds to StringRefs for string literals.
 struct StringLiteralValueId : public IdBase<StringLiteralValueId> {
   static constexpr llvm::StringLiteral Label = "string";
-  using ValueType = llvm::StringRef;
   static const StringLiteralValueId None;
   using IdBase::IdBase;
 };

toolchain/base/value_store_types.h

@@ -13,16 +13,10 @@
 namespace Carbon {
 
 // Common calculation for ValueStore types.
-template <typename IdT, typename ValueT = IdT::ValueType,
-          typename KeyT = ValueT>
+template <typename IdT, typename ValueT = IdT::ValueType>
 class ValueStoreTypes {
  public:
-  using ValueType = std::decay_t<ValueT>;
-
-  // TODO: Would be a bit cleaner to not have this here as it's only meaningful
-  // to the `CanonicalValueStore`, not to other `ValueStore`s. Planned to fix
-  // with a larger refactoring.
-  using KeyType = std::decay_t<KeyT>;
+  using ValueType = std::remove_cvref_t<ValueT>;
 
   // Typically we want to use `ValueType&` and `const ValueType& to avoid
   // copies, but when the value type is a `StringRef`, we assume external
@@ -35,14 +29,6 @@ class ValueStoreTypes {
                          llvm::StringRef, const ValueType&>;
 };
 
-// If `IdT` provides a distinct `IdT::KeyType`, default to that for the key
-// type.
-template <typename IdT>
-  requires(!std::same_as<typename IdT::ValueType, typename IdT::KeyType>)
-class ValueStoreTypes<IdT>
-    : public ValueStoreTypes<IdT, typename IdT::ValueType,
-                             typename IdT::KeyType> {};
-
 }  // namespace Carbon
 
 #endif  // CARBON_TOOLCHAIN_BASE_VALUE_STORE_TYPES_H_

toolchain/check/context.h

@@ -28,11 +28,13 @@
 #include "toolchain/parse/node_ids.h"
 #include "toolchain/parse/tree.h"
 #include "toolchain/parse/tree_and_subtrees.h"
+#include "toolchain/sem_ir/facet_type_info.h"
 #include "toolchain/sem_ir/file.h"
 #include "toolchain/sem_ir/ids.h"
 #include "toolchain/sem_ir/import_ir.h"
 #include "toolchain/sem_ir/inst.h"
 #include "toolchain/sem_ir/name_scope.h"
+#include "toolchain/sem_ir/specific_interface.h"
 #include "toolchain/sem_ir/typed_insts.h"
 
 namespace Carbon::Check {
@@ -246,15 +248,14 @@ class Context {
   auto associated_constants() -> SemIR::AssociatedConstantStore& {
     return sem_ir().associated_constants();
   }
-  auto facet_types() -> CanonicalValueStore<SemIR::FacetTypeId>& {
+  auto facet_types() -> SemIR::FacetTypeInfoStore& {
     return sem_ir().facet_types();
   }
   auto identified_facet_types() -> SemIR::File::IdentifiedFacetTypeStore& {
     return sem_ir().identified_facet_types();
   }
   auto impls() -> SemIR::ImplStore& { return sem_ir().impls(); }
-  auto specific_interfaces()
-      -> CanonicalValueStore<SemIR::SpecificInterfaceId>& {
+  auto specific_interfaces() -> SemIR::SpecificInterfaceStore& {
     return sem_ir().specific_interfaces();
   }
   auto generics() -> SemIR::GenericStore& { return sem_ir().generics(); }

toolchain/check/eval.cpp

@@ -24,6 +24,7 @@
 #include "toolchain/diagnostics/format_providers.h"
 #include "toolchain/sem_ir/builtin_function_kind.h"
 #include "toolchain/sem_ir/constant.h"
+#include "toolchain/sem_ir/facet_type_info.h"
 #include "toolchain/sem_ir/function.h"
 #include "toolchain/sem_ir/generic.h"
 #include "toolchain/sem_ir/id_kind.h"
@@ -175,11 +176,10 @@ class EvalContext {
   auto interfaces() -> const SemIR::InterfaceStore& {
     return sem_ir().interfaces();
   }
-  auto specific_interfaces()
-      -> CanonicalValueStore<SemIR::SpecificInterfaceId>& {
+  auto specific_interfaces() -> SemIR::SpecificInterfaceStore& {
     return sem_ir().specific_interfaces();
   }
-  auto facet_types() -> CanonicalValueStore<SemIR::FacetTypeId>& {
+  auto facet_types() -> SemIR::FacetTypeInfoStore& {
     return sem_ir().facet_types();
   }
   auto generics() -> const SemIR::GenericStore& { return sem_ir().generics(); }

toolchain/sem_ir/BUILD

@@ -32,6 +32,7 @@ cc_library(
         "inst_categories.h",
         "inst_kind.h",
         "singleton_insts.h",
+        "specific_interface.h",
         "typed_insts.h",
     ],
     textual_hdrs = ["inst_kind.def"],
@@ -40,6 +41,7 @@ cc_library(
         "//common:enum_base",
         "//common:ostream",
         "//common:type_enum",
+        "//toolchain/base:canonical_value_store",
         "//toolchain/base:index_base",
         "//toolchain/base:int",
         "//toolchain/base:value_ids",
@@ -67,6 +69,7 @@ cc_library(
         ":typed_insts",
         "//common:hashtable_key_context",
         "//common:ostream",
+        "//toolchain/base:canonical_value_store",
         "@llvm-project//clang:ast",
     ],
 )

toolchain/sem_ir/clang_decl.h

@@ -7,6 +7,7 @@
 
 #include "common/hashtable_key_context.h"
 #include "common/ostream.h"
+#include "toolchain/base/canonical_value_store.h"
 #include "toolchain/sem_ir/ids.h"
 
 // NOLINTNEXTLINE(readability-identifier-naming)
@@ -73,14 +74,13 @@ struct ClangDecl : public Printable<ClangDecl> {
 struct ClangDeclId : public IdBase<ClangDeclId> {
   static constexpr llvm::StringLiteral Label = "clang_decl_id";
 
-  using ValueType = ClangDecl;
-
-  // Use the AST node pointer directly when doing `Lookup` to find an ID.
-  using KeyType = clang::Decl*;
-
   using IdBase::IdBase;
 };
 
+// Use the AST node pointer directly when doing `Lookup` to find an ID.
+using ClangDeclStore =
+    CanonicalValueStore<ClangDeclId, clang::Decl*, ClangDecl>;
+
 }  // namespace Carbon::SemIR
 
 #endif  // CARBON_TOOLCHAIN_SEM_IR_CLANG_DECL_H_

toolchain/sem_ir/facet_type_info.h

@@ -9,6 +9,7 @@
 #include "llvm/ADT/StringExtras.h"
 #include "toolchain/base/canonical_value_store.h"
 #include "toolchain/sem_ir/ids.h"
+#include "toolchain/sem_ir/specific_interface.h"
 
 namespace Carbon::SemIR {
 
@@ -86,6 +87,8 @@ constexpr FacetTypeInfo::RewriteConstraint
     FacetTypeInfo::RewriteConstraint::None = {.lhs_id = InstId::None,
                                               .rhs_id = InstId::None};
 
+using FacetTypeInfoStore = CanonicalValueStore<FacetTypeId, FacetTypeInfo>;
+
 struct IdentifiedFacetType {
   using RequiredInterface = SpecificInterface;
 

toolchain/sem_ir/file.h

@@ -34,6 +34,7 @@
 #include "toolchain/sem_ir/name.h"
 #include "toolchain/sem_ir/name_scope.h"
 #include "toolchain/sem_ir/singleton_insts.h"
+#include "toolchain/sem_ir/specific_interface.h"
 #include "toolchain/sem_ir/struct_type_field.h"
 #include "toolchain/sem_ir/type.h"
 #include "toolchain/sem_ir/type_info.h"
@@ -162,12 +163,8 @@ class File : public Printable<File> {
     return associated_constants_;
   }
   // TODO: Rename these to `facet_type_infos`.
-  auto facet_types() -> CanonicalValueStore<FacetTypeId>& {
-    return facet_types_;
-  }
-  auto facet_types() const -> const CanonicalValueStore<FacetTypeId>& {
-    return facet_types_;
-  }
+  auto facet_types() -> FacetTypeInfoStore& { return facet_types_; }
+  auto facet_types() const -> const FacetTypeInfoStore& { return facet_types_; }
   auto identified_facet_types() -> IdentifiedFacetTypeStore& {
     return identified_facet_types_;
   }
@@ -176,11 +173,10 @@ class File : public Printable<File> {
   }
   auto impls() -> ImplStore& { return impls_; }
   auto impls() const -> const ImplStore& { return impls_; }
-  auto specific_interfaces() -> CanonicalValueStore<SpecificInterfaceId>& {
+  auto specific_interfaces() -> SpecificInterfaceStore& {
     return specific_interfaces_;
   }
-  auto specific_interfaces() const
-      -> const CanonicalValueStore<SpecificInterfaceId>& {
+  auto specific_interfaces() const -> const SpecificInterfaceStore& {
     return specific_interfaces_;
   }
   auto generics() -> GenericStore& { return generics_; }
@@ -201,12 +197,8 @@ class File : public Printable<File> {
   // pointer in the constructor and remove this function. This is part of
   // https://github.com/carbon-language/carbon-lang/issues/4666
   auto set_cpp_ast(clang::ASTUnit* cpp_ast) -> void { cpp_ast_ = cpp_ast; }
-  auto clang_decls() -> CanonicalValueStore<ClangDeclId>& {
-    return clang_decls_;
-  }
-  auto clang_decls() const -> const CanonicalValueStore<ClangDeclId>& {
-    return clang_decls_;
-  }
+  auto clang_decls() -> ClangDeclStore& { return clang_decls_; }
+  auto clang_decls() const -> const ClangDeclStore& { return clang_decls_; }
   auto names() const -> NameStoreWrapper {
     return NameStoreWrapper(&identifiers());
   }
@@ -303,7 +295,7 @@ class File : public Printable<File> {
   AssociatedConstantStore associated_constants_;
 
   // Storage for facet types.
-  CanonicalValueStore<FacetTypeId> facet_types_;
+  FacetTypeInfoStore facet_types_;
 
   // Storage for identified facet types.
   IdentifiedFacetTypeStore identified_facet_types_;
@@ -313,7 +305,7 @@ class File : public Printable<File> {
 
   // Storage for specific interfaces, which are an individual unit of impl
   // lookup for a single interface.
-  CanonicalValueStore<SpecificInterfaceId> specific_interfaces_;
+  SpecificInterfaceStore specific_interfaces_;
 
   // Storage for generics.
   GenericStore generics_;
@@ -338,7 +330,7 @@ class File : public Printable<File> {
   // Clang AST declarations pointing to the AST and their mapped Carbon
   // instructions. When calling `Lookup()`, `inst_id` is ignored. `Add()` will
   // not add multiple entries with the same `decl` and different `inst_id`.
-  CanonicalValueStore<ClangDeclId> clang_decls_;
+  ClangDeclStore clang_decls_;
 
   // All instructions. The first entries will always be the singleton
   // instructions.

toolchain/sem_ir/ids.h

@@ -16,10 +16,8 @@
 
 namespace Carbon::SemIR {
 
-// Forward declare indexed types, for integration with ValueStore.
+// TODO: This is in use, but not here.
 class File;
-struct FacetTypeInfo;
-struct SpecificInterface;
 
 // The ID of an `Inst`.
 struct InstId : public IdBase<InstId> {
@@ -288,10 +286,9 @@ struct AssociatedConstantId : public IdBase<AssociatedConstantId> {
   using IdBase::IdBase;
 };
 
-// The ID of an facet type value.
+// The ID of a `FacetTypeInfo`.
 struct FacetTypeId : public IdBase<FacetTypeId> {
   static constexpr llvm::StringLiteral Label = "facet_type";
-  using ValueType = FacetTypeInfo;
 
   using IdBase::IdBase;
 };
@@ -329,12 +326,11 @@ struct SpecificId : public IdBase<SpecificId> {
   using IdBase::IdBase;
 };
 
-// The ID of a SpecificInterface, which is an interface and a specific pair.
+// The ID of a `SpecificInterface`, which is an interface and a specific pair.
 struct SpecificInterfaceId : public IdBase<SpecificInterfaceId> {
   using DiagnosticType = Diagnostics::TypeInfo<std::string>;
 
   static constexpr llvm::StringLiteral Label = "specific_interface";
-  using ValueType = SpecificInterface;
 
   using IdBase::IdBase;
 };
@@ -948,22 +944,6 @@ struct AnyRawId : public AnyIdBase {
   constexpr explicit AnyRawId(int32_t id) : AnyIdBase(id) {}
 };
 
-// A pair of an interface and a specific for that interface.
-struct SpecificInterface {
-  using DiagnosticType = Diagnostics::TypeInfo<std::string>;
-
-  InterfaceId interface_id;
-  SpecificId specific_id;
-
-  static const SpecificInterface None;
-
-  friend auto operator==(const SpecificInterface& lhs,
-                         const SpecificInterface& rhs) -> bool = default;
-};
-
-constexpr SpecificInterface SpecificInterface::None = {
-    .interface_id = InterfaceId::None, .specific_id = SpecificId::None};
-
 }  // namespace Carbon::SemIR
 
 #endif  // CARBON_TOOLCHAIN_SEM_IR_IDS_H_

toolchain/sem_ir/specific_interface.h

@@ -0,0 +1,34 @@
+// 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
+
+#ifndef CARBON_TOOLCHAIN_SEM_IR_SPECIFIC_INTERFACE_H_
+#define CARBON_TOOLCHAIN_SEM_IR_SPECIFIC_INTERFACE_H_
+
+#include "toolchain/base/canonical_value_store.h"
+#include "toolchain/sem_ir/ids.h"
+
+namespace Carbon::SemIR {
+
+// A pair of an interface and a specific for that interface.
+struct SpecificInterface {
+  using DiagnosticType = Diagnostics::TypeInfo<std::string>;
+
+  InterfaceId interface_id;
+  SpecificId specific_id;
+
+  static const SpecificInterface None;
+
+  friend auto operator==(const SpecificInterface& lhs,
+                         const SpecificInterface& rhs) -> bool = default;
+};
+
+constexpr SpecificInterface SpecificInterface::None = {
+    .interface_id = InterfaceId::None, .specific_id = SpecificId::None};
+
+using SpecificInterfaceStore =
+    CanonicalValueStore<SpecificInterfaceId, SpecificInterface>;
+
+}  // namespace Carbon::SemIR
+
+#endif  // CARBON_TOOLCHAIN_SEM_IR_SPECIFIC_INTERFACE_H_

toolchain/sem_ir/typed_insts.h

@@ -10,6 +10,7 @@
 #include "toolchain/sem_ir/ids.h"
 #include "toolchain/sem_ir/inst_kind.h"
 #include "toolchain/sem_ir/singleton_insts.h"
+#include "toolchain/sem_ir/specific_interface.h"
 
 // Representations for specific kinds of instructions.
 //