Make FacetTypeInfo and CompleteFacetType stores share id indices (#4989)

The CompleteFacetType value store is now a RelationalValueStore. This
type of store uses some _other_ id as the key for insertion. It allows
checking if values of the _other_ id are present in the store, since
those ids are handed out by another store and will (necessarily) exist
before there is a matching value in the RelationalValueStore.

The lookup for precense of the _other_ id returns the id of a value in
the RelationalValueStore. That id can be used to get the value out of
the store.

For our use case, the RelationalValueStore maps from FacetTypeId to
CompleteFacetTypeId. So you add a CompleteFacetType to the store with a
FacetTypeId. Then you can query with a FacetTypeId to see if there
exists a CompleteFacetTypeId. And if there is, you can use that
CompleteFacetTypeId thereafter to get the CompleteFacetType value from
the store.

This removes the need for ValueStore::GetMutable() and removes the
method. FacetTypeInfo no longer has a field that needs to be carefully
excluded from hash and comparison.

toolchain/base/value_store.h

@@ -5,6 +5,7 @@
 #ifndef CARBON_TOOLCHAIN_BASE_VALUE_STORE_H_
 #define CARBON_TOOLCHAIN_BASE_VALUE_STORE_H_
 
+#include <memory>
 #include <type_traits>
 
 #include "common/check.h"
@@ -146,9 +147,6 @@ class CanonicalValueStore {
   // Returns the value for an ID.
   auto Get(IdT id) const -> ConstRefType { return values_.Get(id); }
 
-  // Returns the value for an ID. Changes should not affect hash or ==.
-  auto GetMutable(IdT id) -> RefType { return values_.Get(id); }
-
   // Looks up the canonical ID for a value, or returns `None` if not in the
   // store.
   auto Lookup(ValueType value) const -> IdT;
@@ -223,6 +221,70 @@ auto CanonicalValueStore<IdT>::Reserve(size_t size) -> void {
   values_.Reserve(size);
 }
 
+// A ValueStore that builds a 1:1 relationship between two IDs.
+// * `RelatedIdT` represents a related ID that can be used to find values in the
+//   store.
+// * `IdT` is the actual ID of values in this store, and `IdT::ValueType` is the
+//   value type being stored.
+//
+// The value store builds a mapping so that either ID can be used later to find
+// a value. And the user can query if a related `RelatedIdT` has been used to
+// add a value to the store or not.
+//
+// When adding to the store, the user provides the related `RelatedIdT` along
+// with the value being stored, and gets back the ID of the value in the store.
+//
+// This store requires more storage space than normal ValueStore does, as it
+// requires storing a bit for presence of each `RelatedIdT`. And it allocates
+// memory for values for all IDs up largest ID present in the store, even if
+// they are not yet used.
+template <typename RelatedIdT, typename IdT>
+class RelationalValueStore {
+ public:
+  using ValueType = IdT::ValueType;
+  using ConstRefType = ValueStore<IdT>::ConstRefType;
+
+  // Given the related ID and a value, stores the value and returns a mapped ID
+  // to reference it in the store.
+  auto Add(RelatedIdT related_id, ValueType value) -> IdT {
+    CARBON_DCHECK(related_id.index >= 0, "{0}", related_id);
+    IdT id(related_id.index);
+    if (static_cast<size_t>(id.index) >= values_.size()) {
+      values_.resize(id.index + 1);
+    }
+    auto& opt = values_[id.index];
+    CARBON_CHECK(!opt.has_value(),
+                 "Add with `related_id` that was already added to the store");
+    opt.emplace(std::move(value));
+    return id;
+  }
+
+  // Returns the ID of a value in the store if the `related_id` was previously
+  // used to add a value to the store, or None.
+  auto TryGetId(RelatedIdT related_id) const -> IdT {
+    CARBON_DCHECK(related_id.index >= 0, "{0}", related_id);
+    if (static_cast<size_t>(related_id.index) >= values_.size()) {
+      return IdT::None;
+    }
+    auto& opt = values_[related_id.index];
+    if (!opt.has_value()) {
+      return IdT::None;
+    }
+    return IdT(related_id.index);
+  }
+
+  // Returns a value for an ID.
+  auto Get(IdT id) const -> ConstRefType {
+    CARBON_DCHECK(id.index >= 0, "{0}", id);
+    return *values_[id.index];
+  }
+
+ private:
+  // Set inline size to 0 because these will typically be too large for the
+  // stack, while this does make File smaller.
+  llvm::SmallVector<std::optional<std::decay_t<ValueType>>, 0> values_;
+};
+
 }  // namespace Carbon
 
 #endif  // CARBON_TOOLCHAIN_BASE_VALUE_STORE_H_

toolchain/check/context.h

@@ -224,7 +224,7 @@ class Context {
   auto facet_types() -> CanonicalValueStore<SemIR::FacetTypeId>& {
     return sem_ir().facet_types();
   }
-  auto complete_facet_types() -> ValueStore<SemIR::CompleteFacetTypeId>& {
+  auto complete_facet_types() -> SemIR::File::CompleteFacetTypeStore& {
     return sem_ir().complete_facet_types();
   }
   auto impls() -> SemIR::ImplStore& { return sem_ir().impls(); }

toolchain/check/type_completion.cpp

@@ -10,6 +10,7 @@
 #include "toolchain/check/inst.h"
 #include "toolchain/check/type.h"
 #include "toolchain/diagnostics/format_providers.h"
+#include "toolchain/sem_ir/ids.h"
 
 namespace Carbon::Check {
 
@@ -574,9 +575,11 @@ auto RequireConcreteType(Context& context, SemIR::TypeId type_id,
 }
 
 static auto AddCompleteFacetType(Context& context, SemIR::LocId loc_id,
-                                 const SemIR::FacetTypeInfo& facet_type_info,
+                                 SemIR::FacetTypeId facet_type_id,
                                  FacetTypeContext context_for_diagnostics)
     -> SemIR::CompleteFacetTypeId {
+  const auto& facet_type_info = context.facet_types().Get(facet_type_id);
+
   SemIR::CompleteFacetType result;
   result.required_interfaces.reserve(facet_type_info.impls_constraints.size());
   // Every mentioned interface needs to be defined.
@@ -611,7 +614,7 @@ static auto AddCompleteFacetType(Context& context, SemIR::LocId loc_id,
   // TODO: Distinguish interfaces that are required but would not be
   // implemented, such as those from `where .Self impls I`.
   result.num_to_impl = result.required_interfaces.size();
-  return context.complete_facet_types().Add(result);
+  return context.complete_facet_types().Add(facet_type_id, result);
 }
 
 // TODO: RequireCompleteType should do these checks, this should just return
@@ -628,13 +631,13 @@ auto RequireCompleteFacetType(Context& context, SemIR::TypeId type_id,
     return SemIR::CompleteFacetTypeId::None;
   }
 
-  auto& facet_type_info =
-      context.facet_types().GetMutable(facet_type.facet_type_id);
-  if (!facet_type_info.complete_id.has_value()) {
-    facet_type_info.complete_id = AddCompleteFacetType(
-        context, loc_id, facet_type_info, context_for_diagnostics);
+  auto complete_id =
+      context.complete_facet_types().TryGetId(facet_type.facet_type_id);
+  if (!complete_id.has_value()) {
+    complete_id = AddCompleteFacetType(
+        context, loc_id, facet_type.facet_type_id, context_for_diagnostics);
   }
-  return facet_type_info.complete_id;
+  return complete_id;
 }
 
 auto AsCompleteType(Context& context, SemIR::TypeId type_id,

toolchain/sem_ir/dump.cpp

@@ -99,9 +99,10 @@ LLVM_DUMP_METHOD auto Dump(const File& file, FacetTypeId facet_type_id)
       llvm::errs() << "  - ";
       Dump(file, rewrite.rhs_const_id);
     }
-    if (facet_type.complete_id.has_value()) {
+    if (auto complete_id = file.complete_facet_types().TryGetId(facet_type_id);
+        complete_id.has_value()) {
       llvm::errs() << "complete: ";
-      Dump(file, facet_type.complete_id);
+      Dump(file, complete_id);
     }
   } else {
     llvm::errs() << '\n';

toolchain/sem_ir/facet_type_info.cpp

@@ -27,7 +27,6 @@ static auto RewriteLess(const FacetTypeInfo::RewriteConstraint& lhs,
 }
 
 auto FacetTypeInfo::Canonicalize() -> void {
-  CARBON_CHECK(!complete_id.has_value());
   SortAndDeduplicate(impls_constraints, ImplsLess);
   SortAndDeduplicate(rewrite_constraints, RewriteLess);
 }
@@ -59,9 +58,6 @@ auto FacetTypeInfo::Print(llvm::raw_ostream& out) const -> void {
     out << outer_sep << "+ TODO requirements";
   }
 
-  if (complete_id.has_value()) {
-    out << outer_sep << "complete: " << complete_id;
-  }
   out << "}";
 }
 

toolchain/sem_ir/facet_type_info.h

@@ -56,12 +56,6 @@ struct FacetTypeInfo : Printable<FacetTypeInfo> {
   // TODO: Remove once all requirements are supported.
   bool other_requirements;
 
-  // This is should be `None` for new facet type values, and only set as a
-  // private implementation detail of `RequireCompleteFacetType`. It is stored
-  // here so that we only compute its value once per facet type. This is not
-  // part of the value of the facet type, excluded from `==` and its hash value.
-  CompleteFacetTypeId complete_id = CompleteFacetTypeId::None;
-
   // Sorts and deduplicates constraints. Call after building the value, and then
   // don't mutate this value afterwards.
   auto Canonicalize() -> void;

toolchain/sem_ir/file.h

@@ -57,6 +57,9 @@ struct ExprRegion {
 // Provides semantic analysis on a Parse::Tree.
 class File : public Printable<File> {
  public:
+  using CompleteFacetTypeStore =
+      RelationalValueStore<SemIR::FacetTypeId, SemIR::CompleteFacetTypeId>;
+
   // Starts a new file for Check::CheckParseTree.
   explicit File(const Parse::Tree* parse_tree, CheckIRId check_ir_id,
                 const std::optional<Parse::Tree::PackagingDecl>& packaging_decl,
@@ -156,10 +159,10 @@ class File : public Printable<File> {
   auto facet_types() const -> const CanonicalValueStore<FacetTypeId>& {
     return facet_types_;
   }
-  auto complete_facet_types() -> ValueStore<CompleteFacetTypeId>& {
+  auto complete_facet_types() -> CompleteFacetTypeStore& {
     return complete_facet_types_;
   }
-  auto complete_facet_types() const -> const ValueStore<CompleteFacetTypeId>& {
+  auto complete_facet_types() const -> const CompleteFacetTypeStore& {
     return complete_facet_types_;
   }
   auto impls() -> ImplStore& { return impls_; }
@@ -276,7 +279,7 @@ class File : public Printable<File> {
   CanonicalValueStore<FacetTypeId> facet_types_;
 
   // Storage for complete facet types.
-  ValueStore<CompleteFacetTypeId> complete_facet_types_;
+  CompleteFacetTypeStore complete_facet_types_;
 
   // Storage for impls.
   ImplStore impls_;

toolchain/sem_ir/ids.h

@@ -275,7 +275,7 @@ struct AssociatedConstantId : public IdBase<AssociatedConstantId> {
 constexpr AssociatedConstantId AssociatedConstantId::None =
     AssociatedConstantId(NoneIndex);
 
-// The ID of an faceet type value.
+// The ID of an facet type value.
 struct FacetTypeId : public IdBase<FacetTypeId> {
   static constexpr llvm::StringLiteral Label = "facet_type";
   using ValueType = FacetTypeInfo;
@@ -288,7 +288,7 @@ struct FacetTypeId : public IdBase<FacetTypeId> {
 
 constexpr FacetTypeId FacetTypeId::None = FacetTypeId(NoneIndex);
 
-// The ID of an resolved faceet type value.
+// The ID of an resolved facet type value.
 struct CompleteFacetTypeId : public IdBase<CompleteFacetTypeId> {
   static constexpr llvm::StringLiteral Label = "complete_facet_type";
   using ValueType = CompleteFacetType;