Refactor ValueStore to reduce template type repetition. (#3437)

There's a trade-off here of explicitness in the use versus repetition,
but I'm hoping the Id offers sufficient info (also, some Ids already
relied on this, so this builds consistency). The forward declarations
I'm mixed on, but they are difficult to avoid if heading down this route
due to interdependencies between ids and types which contain ids.

toolchain/base/value_store.h

@@ -49,7 +49,7 @@ class Real : public Printable<Real> {
 
 // Corresponds to an integer value represented by an APInt.
 struct IntId : public IdBase, public Printable<IntId> {
-  using IndexedType = const llvm::APInt;
+  using ValueType = const llvm::APInt;
   static const IntId Invalid;
   using IdBase::IdBase;
   auto Print(llvm::raw_ostream& out) const -> void {
@@ -61,7 +61,7 @@ constexpr IntId IntId::Invalid(IntId::InvalidIndex);
 
 // Corresponds to a Real value.
 struct RealId : public IdBase, public Printable<RealId> {
-  using IndexedType = const Real;
+  using ValueType = const Real;
   static const RealId Invalid;
   using IdBase::IdBase;
   auto Print(llvm::raw_ostream& out) const -> void {
@@ -73,7 +73,7 @@ constexpr RealId RealId::Invalid(RealId::InvalidIndex);
 
 // Corresponds to a StringRef.
 struct StringId : public IdBase, public Printable<StringId> {
-  using IndexedType = const std::string;
+  using ValueType = const std::string;
   static const StringId Invalid;
   using IdBase::IdBase;
   auto Print(llvm::raw_ostream& out) const -> void {
@@ -117,14 +117,19 @@ class ValueStoreNotPrintable {};
 
 // A simple wrapper for accumulating values, providing IDs to later retrieve the
 // value. This does not do deduplication.
-template <typename IdT, typename ValueT = typename IdT::IndexedType>
+//
+// IdT::ValueType must represent the type being indexed.
+template <typename IdT>
 class ValueStore
-    : public std::conditional<std::is_base_of_v<Printable<ValueT>, ValueT>,
-                              Yaml::Printable<ValueStore<IdT, ValueT>>,
-                              Internal::ValueStoreNotPrintable> {
+    : public std::conditional<
+          std::is_base_of_v<Printable<typename IdT::ValueType>,
+                            typename IdT::ValueType>,
+          Yaml::Printable<ValueStore<IdT>>, Internal::ValueStoreNotPrintable> {
  public:
+  using ValueType = typename IdT::ValueType;
+
   // Stores the value and returns an ID to reference it.
-  auto Add(ValueT value) -> IdT {
+  auto Add(ValueType value) -> IdT {
     IdT id = IdT(values_.size());
     CARBON_CHECK(id.index >= 0) << "Id overflow";
     values_.push_back(std::move(value));
@@ -139,13 +144,13 @@ class ValueStore
   }
 
   // Returns a mutable value for an ID.
-  auto Get(IdT id) -> ValueT& {
+  auto Get(IdT id) -> ValueType& {
     CARBON_CHECK(id.index >= 0) << id.index;
     return values_[id.index];
   }
 
   // Returns the value for an ID.
-  auto Get(IdT id) const -> const ValueT& {
+  auto Get(IdT id) const -> const ValueType& {
     CARBON_CHECK(id.index >= 0) << id.index;
     return values_[id.index];
   }
@@ -163,11 +168,11 @@ class ValueStore
     });
   }
 
-  auto array_ref() const -> llvm::ArrayRef<ValueT> { return values_; }
+  auto array_ref() const -> llvm::ArrayRef<ValueType> { return values_; }
   auto size() const -> int { return values_.size(); }
 
  private:
-  llvm::SmallVector<std::decay_t<ValueT>> values_;
+  llvm::SmallVector<std::decay_t<ValueType>> values_;
 };
 
 // Storage for StringRefs. The caller is responsible for ensuring storage is

toolchain/check/context.h

@@ -302,24 +302,19 @@ class Context {
   auto string_literals() -> StringStoreWrapper<StringLiteralId>& {
     return sem_ir().string_literals();
   }
-  auto functions() -> ValueStore<SemIR::FunctionId, SemIR::Function>& {
+  auto functions() -> ValueStore<SemIR::FunctionId>& {
     return sem_ir().functions();
   }
-  auto classes() -> ValueStore<SemIR::ClassId, SemIR::Class>& {
-    return sem_ir().classes();
-  }
-  auto cross_ref_irs() -> ValueStore<SemIR::CrossRefIRId, const SemIR::File*>& {
+  auto classes() -> ValueStore<SemIR::ClassId>& { return sem_ir().classes(); }
+  auto cross_ref_irs() -> ValueStore<SemIR::CrossRefIRId>& {
     return sem_ir().cross_ref_irs();
   }
   auto names() -> SemIR::NameStoreWrapper { return sem_ir().names(); }
   auto name_scopes() -> SemIR::NameScopeStore& {
     return sem_ir().name_scopes();
   }
-  auto types() -> ValueStore<SemIR::TypeId, SemIR::TypeInfo>& {
-    return sem_ir().types();
-  }
-  auto type_blocks()
-      -> SemIR::BlockValueStore<SemIR::TypeBlockId, SemIR::TypeId>& {
+  auto types() -> ValueStore<SemIR::TypeId>& { return sem_ir().types(); }
+  auto type_blocks() -> SemIR::BlockValueStore<SemIR::TypeBlockId>& {
     return sem_ir().type_blocks();
   }
   auto insts() -> SemIR::InstStore& { return sem_ir().insts(); }

toolchain/sem_ir/file.h

@@ -289,16 +289,12 @@ class File : public Printable<File> {
     return value_stores_->string_literals();
   }
 
-  auto functions() -> ValueStore<FunctionId, Function>& { return functions_; }
-  auto functions() const -> const ValueStore<FunctionId, Function>& {
-    return functions_;
-  }
-  auto classes() -> ValueStore<ClassId, Class>& { return classes_; }
-  auto classes() const -> const ValueStore<ClassId, Class>& { return classes_; }
-  auto cross_ref_irs() -> ValueStore<CrossRefIRId, const File*>& {
-    return cross_ref_irs_;
-  }
-  auto cross_ref_irs() const -> const ValueStore<CrossRefIRId, const File*>& {
+  auto functions() -> ValueStore<FunctionId>& { return functions_; }
+  auto functions() const -> const ValueStore<FunctionId>& { return functions_; }
+  auto classes() -> ValueStore<ClassId>& { return classes_; }
+  auto classes() const -> const ValueStore<ClassId>& { return classes_; }
+  auto cross_ref_irs() -> ValueStore<CrossRefIRId>& { return cross_ref_irs_; }
+  auto cross_ref_irs() const -> const ValueStore<CrossRefIRId>& {
     return cross_ref_irs_;
   }
   auto names() const -> NameStoreWrapper {
@@ -306,12 +302,10 @@ class File : public Printable<File> {
   }
   auto name_scopes() -> NameScopeStore& { return name_scopes_; }
   auto name_scopes() const -> const NameScopeStore& { return name_scopes_; }
-  auto types() -> ValueStore<TypeId, TypeInfo>& { return types_; }
-  auto types() const -> const ValueStore<TypeId, TypeInfo>& { return types_; }
-  auto type_blocks() -> BlockValueStore<TypeBlockId, TypeId>& {
-    return type_blocks_;
-  }
-  auto type_blocks() const -> const BlockValueStore<TypeBlockId, TypeId>& {
+  auto types() -> ValueStore<TypeId>& { return types_; }
+  auto types() const -> const ValueStore<TypeId>& { return types_; }
+  auto type_blocks() -> BlockValueStore<TypeBlockId>& { return type_blocks_; }
+  auto type_blocks() const -> const BlockValueStore<TypeBlockId>& {
     return type_blocks_;
   }
   auto insts() -> InstStore& { return insts_; }
@@ -353,28 +347,28 @@ class File : public Printable<File> {
   std::string filename_;
 
   // Storage for callable objects.
-  ValueStore<FunctionId, Function> functions_;
+  ValueStore<FunctionId> functions_;
 
   // Storage for classes.
-  ValueStore<ClassId, Class> classes_;
+  ValueStore<ClassId> classes_;
 
   // Related IRs. There will always be at least 2 entries, the builtin IR (used
   // for references of builtins) followed by the current IR (used for references
   // crossing instruction blocks).
-  ValueStore<CrossRefIRId, const File*> cross_ref_irs_;
+  ValueStore<CrossRefIRId> cross_ref_irs_;
 
   // Storage for name scopes.
   NameScopeStore name_scopes_;
 
   // Descriptions of types used in this file.
-  ValueStore<TypeId, TypeInfo> types_;
+  ValueStore<TypeId> types_;
 
   // Types that were completed in this file.
   llvm::SmallVector<TypeId> complete_types_;
 
   // Type blocks within the IR. These reference entries in types_. Storage for
   // the data is provided by allocator_.
-  BlockValueStore<TypeBlockId, TypeId> type_blocks_;
+  BlockValueStore<TypeBlockId> type_blocks_;
 
   // All instructions. The first entries will always be cross-references to
   // builtins, at indices matching BuiltinKind ordering.

toolchain/sem_ir/ids.h

@@ -13,8 +13,17 @@
 
 namespace Carbon::SemIR {
 
+// Forward declare indexed types, for integration with ValueStore.
+class File;
+class Inst;
+struct Class;
+struct Function;
+struct TypeInfo;
+
 // The ID of an instruction.
 struct InstId : public IdBase, public Printable<InstId> {
+  using ValueType = Inst;
+
   // An explicitly invalid instruction ID.
   static const InstId Invalid;
 
@@ -52,6 +61,8 @@ constexpr InstId InstId::Invalid = InstId(InstId::InvalidIndex);
 
 // The ID of a function.
 struct FunctionId : public IdBase, public Printable<FunctionId> {
+  using ValueType = Function;
+
   // An explicitly invalid function ID.
   static const FunctionId Invalid;
 
@@ -66,6 +77,8 @@ constexpr FunctionId FunctionId::Invalid = FunctionId(FunctionId::InvalidIndex);
 
 // The ID of a class.
 struct ClassId : public IdBase, public Printable<ClassId> {
+  using ValueType = Class;
+
   // An explicitly invalid class ID.
   static const ClassId Invalid;
 
@@ -80,6 +93,8 @@ constexpr ClassId ClassId::Invalid = ClassId(ClassId::InvalidIndex);
 
 // The ID of a cross-referenced IR.
 struct CrossRefIRId : public IdBase, public Printable<CrossRefIRId> {
+  using ValueType = const File*;
+
   static const CrossRefIRId Builtins;
   using IdBase::IdBase;
   auto Print(llvm::raw_ostream& out) const -> void {
@@ -164,6 +179,8 @@ constexpr NameId NameId::ReturnSlot = NameId(NameId::InvalidIndex - 3);
 
 // The ID of a name scope.
 struct NameScopeId : public IdBase, public Printable<NameScopeId> {
+  using ValueType = llvm::DenseMap<NameId, InstId>;
+
   // An explicitly invalid ID.
   static const NameScopeId Invalid;
 
@@ -179,6 +196,9 @@ constexpr NameScopeId NameScopeId::Invalid =
 
 // The ID of an instruction block.
 struct InstBlockId : public IdBase, public Printable<InstBlockId> {
+  using ElementType = InstId;
+  using ValueType = llvm::MutableArrayRef<ElementType>;
+
   // All File instances must provide the 0th instruction block as empty.
   static const InstBlockId Empty;
 
@@ -207,6 +227,8 @@ constexpr InstBlockId InstBlockId::Unreachable =
 
 // The ID of a type.
 struct TypeId : public IdBase, public Printable<TypeId> {
+  using ValueType = TypeInfo;
+
   // The builtin TypeType.
   static const TypeId TypeType;
 
@@ -235,6 +257,9 @@ constexpr TypeId TypeId::Invalid = TypeId(TypeId::InvalidIndex);
 
 // The ID of a type block.
 struct TypeBlockId : public IdBase, public Printable<TypeBlockId> {
+  using ElementType = TypeId;
+  using ValueType = llvm::MutableArrayRef<ElementType>;
+
   using IdBase::IdBase;
   auto Print(llvm::raw_ostream& out) const -> void {
     out << "typeBlock";

toolchain/sem_ir/value_stores.h

@@ -42,7 +42,7 @@ class InstStore {
   auto size() const -> int { return values_.size(); }
 
  private:
-  ValueStore<InstId, Inst> values_;
+  ValueStore<InstId> values_;
 };
 
 // Provides storage for instructions representing global constants.
@@ -121,7 +121,7 @@ class NameScopeStore {
   }
 
  private:
-  ValueStore<NameScopeId, llvm::DenseMap<NameId, InstId>> values_;
+  ValueStore<NameScopeId> values_;
 };
 
 // Provides a block-based ValueStore, which uses slab allocation of added
@@ -130,22 +130,33 @@ class NameScopeStore {
 //
 // BlockValueStore is used as-is, but there are also children that expose the
 // protected members for type-specific functionality.
-template <typename IdT, typename ValueT>
-class BlockValueStore : public Yaml::Printable<BlockValueStore<IdT, ValueT>> {
+//
+// On IdT, this requires:
+//   - IdT::ElementType to represent the underlying type in the block.
+//   - IdT::ValueType to be llvm::MutableArrayRef<IdT::ElementType> for
+//     compatibility with ValueStore.
+template <typename IdT>
+class BlockValueStore : public Yaml::Printable<BlockValueStore<IdT>> {
  public:
+  using ElementType = typename IdT::ElementType;
+
   explicit BlockValueStore(llvm::BumpPtrAllocator& allocator)
       : allocator_(&allocator) {}
 
   // Adds a block with the given content, returning an ID to reference it.
-  auto Add(llvm::ArrayRef<ValueT> content) -> IdT {
+  auto Add(llvm::ArrayRef<ElementType> content) -> IdT {
     return values_.Add(AllocateCopy(content));
   }
 
   // Returns the requested block.
-  auto Get(IdT id) const -> llvm::ArrayRef<ValueT> { return values_.Get(id); }
+  auto Get(IdT id) const -> llvm::ArrayRef<ElementType> {
+    return values_.Get(id);
+  }
 
   // Returns the requested block.
-  auto Get(IdT id) -> llvm::MutableArrayRef<ValueT> { return values_.Get(id); }
+  auto Get(IdT id) -> llvm::MutableArrayRef<ElementType> {
+    return values_.Get(id);
+  }
 
   auto OutputYaml() const -> Yaml::OutputMapping {
     return Yaml::OutputMapping([&](Yaml::OutputMapping::Map map) {
@@ -184,31 +195,31 @@ class BlockValueStore : public Yaml::Printable<BlockValueStore<IdT, ValueT>> {
  private:
   // Allocates an uninitialized array using our slab allocator.
   auto AllocateUninitialized(std::size_t size)
-      -> llvm::MutableArrayRef<ValueT> {
+      -> llvm::MutableArrayRef<ElementType> {
     // We're not going to run a destructor, so ensure that's OK.
-    static_assert(std::is_trivially_destructible_v<ValueT>);
+    static_assert(std::is_trivially_destructible_v<ElementType>);
 
-    auto storage = static_cast<ValueT*>(
-        allocator_->Allocate(size * sizeof(ValueT), alignof(ValueT)));
-    return llvm::MutableArrayRef<ValueT>(storage, size);
+    auto storage = static_cast<ElementType*>(
+        allocator_->Allocate(size * sizeof(ElementType), alignof(ElementType)));
+    return llvm::MutableArrayRef<ElementType>(storage, size);
   }
 
   // Allocates a copy of the given data using our slab allocator.
-  auto AllocateCopy(llvm::ArrayRef<ValueT> data)
-      -> llvm::MutableArrayRef<ValueT> {
+  auto AllocateCopy(llvm::ArrayRef<ElementType> data)
+      -> llvm::MutableArrayRef<ElementType> {
     auto result = AllocateUninitialized(data.size());
     std::uninitialized_copy(data.begin(), data.end(), result.begin());
     return result;
   }
 
   llvm::BumpPtrAllocator* allocator_;
-  ValueStore<IdT, llvm::MutableArrayRef<ValueT>> values_;
+  ValueStore<IdT> values_;
 };
 
 // Adapts BlockValueStore for instruction blocks.
-class InstBlockStore : public BlockValueStore<InstBlockId, InstId> {
+class InstBlockStore : public BlockValueStore<InstBlockId> {
  public:
-  using BaseType = BlockValueStore<InstBlockId, InstId>;
+  using BaseType = BlockValueStore<InstBlockId>;
 
   using BaseType::AddDefaultValue;
   using BaseType::AddUninitialized;
@@ -216,7 +227,7 @@ class InstBlockStore : public BlockValueStore<InstBlockId, InstId> {
 
   auto Set(InstBlockId block_id, llvm::ArrayRef<InstId> content) -> void {
     CARBON_CHECK(block_id != InstBlockId::Unreachable);
-    BlockValueStore<InstBlockId, InstId>::Set(block_id, content);
+    BlockValueStore<InstBlockId>::Set(block_id, content);
   }
 };