diff --git a/toolchain/base/value_store.h b/toolchain/base/value_store.h index d598e4090021..941ca144ce27 100644 --- a/toolchain/base/value_store.h +++ b/toolchain/base/value_store.h @@ -88,39 +88,29 @@ struct RealId : public IdBase, public Printable { }; constexpr RealId RealId::Invalid(RealId::InvalidIndex); -// Corresponds to a StringRef. -struct StringId : public IdBase, public Printable { - using ValueType = std::string; - static const StringId Invalid; - using IdBase::IdBase; - auto Print(llvm::raw_ostream& out) const -> void { - out << "str"; - IdBase::Print(out); - } -}; -constexpr StringId StringId::Invalid(StringId::InvalidIndex); - -// Adapts StringId for identifiers. +// Corresponds to StringRefs for identifiers. // // `NameId` relies on the values of this type other than `Invalid` all being // non-negative. struct IdentifierId : public IdBase, public Printable { + using ValueType = llvm::StringRef; static const IdentifierId Invalid; using IdBase::IdBase; auto Print(llvm::raw_ostream& out) const -> void { - out << "strId"; + out << "identifier"; IdBase::Print(out); } }; constexpr IdentifierId IdentifierId::Invalid(IdentifierId::InvalidIndex); -// Adapts StringId for values of string literals. +// Corresponds to StringRefs for string literals. struct StringLiteralValueId : public IdBase, public Printable { + using ValueType = llvm::StringRef; static const StringLiteralValueId Invalid; using IdBase::IdBase; auto Print(llvm::raw_ostream& out) const -> void { - out << "strLit"; + out << "string"; IdBase::Print(out); } }; @@ -128,6 +118,7 @@ constexpr StringLiteralValueId StringLiteralValueId::Invalid( StringLiteralValueId::InvalidIndex); namespace Internal { + // Used as a parent class for non-printable types. This is just for // std::conditional, not as an API. class ValueStoreNotPrintable {}; @@ -146,6 +137,16 @@ class ValueStore public: using ValueType = typename IdT::ValueType; + // Typically we want to use `ValueType&` and `const ValueType& to avoid + // copies, but when the value type is a `StringRef`, we assume external + // storage for the string data and both our value type and ref type will be + // `StringRef`. This will preclude mutation of the string data. + using RefType = std::conditional_t, + llvm::StringRef, ValueType&>; + using ConstRefType = + std::conditional_t, + llvm::StringRef, const ValueType&>; + // Stores the value and returns an ID to reference it. auto Add(ValueType value) -> IdT { IdT id = IdT(values_.size()); @@ -162,13 +163,13 @@ class ValueStore } // Returns a mutable value for an ID. - auto Get(IdT id) -> ValueType& { + auto Get(IdT id) -> RefType { CARBON_CHECK(id.index >= 0) << id; return values_[id.index]; } // Returns the value for an ID. - auto Get(IdT id) const -> const ValueType& { + auto Get(IdT id) const -> ConstRefType { CARBON_CHECK(id.index >= 0) << id; return values_[id.index]; } @@ -203,12 +204,18 @@ template class CanonicalValueStore { public: using ValueType = typename IdT::ValueType; + using RefType = typename ValueStore::RefType; + using ConstRefType = typename ValueStore::ConstRefType; // Stores a canonical copy of the value and returns an ID to reference it. auto Add(ValueType value) -> IdT; // Returns the value for an ID. - auto Get(IdT id) const -> const ValueType& { return values_.Get(id); } + auto Get(IdT id) const -> ConstRefType { return values_.Get(id); } + + // Looks up the canonical ID for a value, or returns invalid if not in the + // store. + auto Lookup(ValueType value) const -> IdT; // Reserves space. auto Reserve(size_t size) -> void { @@ -259,123 +266,30 @@ auto CanonicalValueStore::Add(ValueType value) -> IdT { return set_.Insert(value, make_key, KeyContext(values_.array_ref())).key(); } -// Storage for StringRefs. The caller is responsible for ensuring storage is -// allocated. -template <> -class CanonicalValueStore - : public Yaml::Printable> { - public: - // Returns an ID to reference the value. May return an existing ID if the - // string was previously added. - auto Add(llvm::StringRef value) -> StringId; - - // Returns the value for an ID. - auto Get(StringId id) const -> llvm::StringRef { - CARBON_CHECK(id.is_valid()); - return values_[id.index]; - } - - // Returns an ID for the value, or Invalid if not found. - auto Lookup(llvm::StringRef value) const -> StringId; - - auto OutputYaml() const -> Yaml::OutputMapping { - return Yaml::OutputMapping([&](Yaml::OutputMapping::Map map) { - for (auto [i, val] : llvm::enumerate(values_)) { - map.Add(PrintToString(StringId(i)), val); - } - }); - } - - auto size() const -> size_t { return values_.size(); } - - private: - class KeyContext; - - // Set inline sizes to 0 because these will typically be too large for the - // stack, while this does make File smaller. - Set set_; - llvm::SmallVector values_; -}; - -class CanonicalValueStore::KeyContext - : public TranslatingKeyContext { - public: - explicit KeyContext(llvm::ArrayRef values) - : values_(values) {} - - auto TranslateKey(StringId id) const -> llvm::StringRef { - return values_[id.index]; - } - - private: - llvm::ArrayRef values_; -}; - -inline auto CanonicalValueStore::Add(llvm::StringRef value) - -> StringId { - auto make_key = [&] { - auto id = static_cast(values_.size()); - CARBON_CHECK(id.index >= 0) << "Too many unique strings"; - values_.push_back(value); - return id; - }; - return set_.Insert(value, make_key, KeyContext(values_)).key(); -} - -inline auto CanonicalValueStore::Lookup(llvm::StringRef value) const - -> StringId { - if (auto result = set_.Lookup(value, KeyContext(values_))) { +template +auto CanonicalValueStore::Lookup(ValueType value) const -> IdT { + if (auto result = set_.Lookup(value, KeyContext(values_.array_ref()))) { return result.key(); } - return StringId::Invalid; + return IdT::Invalid; } -// A thin wrapper around a `ValueStore` that provides a different IdT, -// while using a unified storage for values. This avoids potentially -// duplicative string hash maps, which are expensive. -template -class StringStoreWrapper : public Printable> { - public: - explicit StringStoreWrapper(CanonicalValueStore* values) - : values_(values) {} - - auto Add(llvm::StringRef value) -> IdT { - return IdT(values_->Add(value).index); - } - - auto Get(IdT id) const -> llvm::StringRef { - return values_->Get(StringId(id.index)); - } - - auto Lookup(llvm::StringRef value) const -> IdT { - return IdT(values_->Lookup(value).index); - } - - auto Print(llvm::raw_ostream& out) const -> void { out << *values_; } - - auto size() const -> size_t { return values_->size(); } - - private: - CanonicalValueStore* values_; -}; - using FloatValueStore = CanonicalValueStore; // Stores that will be used across compiler phases for a given compilation unit. // This is provided mainly so that they don't need to be passed separately. class SharedValueStores : public Yaml::Printable { public: - explicit SharedValueStores() - : identifiers_(&strings_), string_literal_values_(&strings_) {} + explicit SharedValueStores() = default; // Not copyable or movable. SharedValueStores(const SharedValueStores&) = delete; auto operator=(const SharedValueStores&) -> SharedValueStores& = delete; - auto identifiers() -> StringStoreWrapper& { + auto identifiers() -> CanonicalValueStore& { return identifiers_; } - auto identifiers() const -> const StringStoreWrapper& { + auto identifiers() const -> const CanonicalValueStore& { return identifiers_; } auto ints() -> CanonicalValueStore& { return ints_; } @@ -384,12 +298,12 @@ class SharedValueStores : public Yaml::Printable { auto reals() const -> const ValueStore& { return reals_; } auto floats() -> FloatValueStore& { return floats_; } auto floats() const -> const FloatValueStore& { return floats_; } - auto string_literal_values() -> StringStoreWrapper& { - return string_literal_values_; + auto string_literal_values() -> CanonicalValueStore& { + return string_literals_; } auto string_literal_values() const - -> const StringStoreWrapper& { - return string_literal_values_; + -> const CanonicalValueStore& { + return string_literals_; } auto OutputYaml(std::optional filename = std::nullopt) const @@ -402,7 +316,8 @@ class SharedValueStores : public Yaml::Printable { Yaml::OutputMapping([&](Yaml::OutputMapping::Map map) { map.Add("ints", ints_.OutputYaml()); map.Add("reals", reals_.OutputYaml()); - map.Add("strings", strings_.OutputYaml()); + map.Add("identifiers", identifiers_.OutputYaml()); + map.Add("strings", string_literals_.OutputYaml()); })); }); } @@ -412,9 +327,8 @@ class SharedValueStores : public Yaml::Printable { ValueStore reals_; FloatValueStore floats_; - CanonicalValueStore strings_; - StringStoreWrapper identifiers_; - StringStoreWrapper string_literal_values_; + CanonicalValueStore identifiers_; + CanonicalValueStore string_literals_; }; } // namespace Carbon diff --git a/toolchain/base/value_store_test.cpp b/toolchain/base/value_store_test.cpp index bf377f5c719a..9608e755f150 100644 --- a/toolchain/base/value_store_test.cpp +++ b/toolchain/base/value_store_test.cpp @@ -77,34 +77,55 @@ TEST(ValueStore, Float) { Eq(llvm::APFloatBase::cmpEqual)); } -TEST(ValueStore, String) { +TEST(ValueStore, Identifiers) { std::string a = "a"; std::string b = "b"; SharedValueStores value_stores; + auto a_id = value_stores.identifiers().Add(a); + auto b_id = value_stores.identifiers().Add(b); + + ASSERT_TRUE(a_id.is_valid()); + ASSERT_TRUE(b_id.is_valid()); + EXPECT_THAT(a_id, Not(Eq(b_id))); + + EXPECT_THAT(value_stores.identifiers().Get(a_id), Eq(a)); + EXPECT_THAT(value_stores.identifiers().Get(b_id), Eq(b)); + + EXPECT_THAT(value_stores.identifiers().Lookup(a), Eq(a_id)); + EXPECT_THAT(value_stores.identifiers().Lookup("c"), + Eq(IdentifierId::Invalid)); +} + +TEST(ValueStore, StringLiterals) { + std::string a = "a"; + std::string b = "b"; + SharedValueStores value_stores; + + auto a_id = value_stores.string_literal_values().Add(a); auto b_id = value_stores.string_literal_values().Add(b); ASSERT_TRUE(a_id.is_valid()); ASSERT_TRUE(b_id.is_valid()); + EXPECT_THAT(a_id, Not(Eq(b_id))); - EXPECT_THAT(a_id.index, Not(Eq(b_id.index))); - EXPECT_THAT(value_stores.identifiers().Get(a_id), Eq(a)); + EXPECT_THAT(value_stores.string_literal_values().Get(a_id), Eq(a)); EXPECT_THAT(value_stores.string_literal_values().Get(b_id), Eq(b)); - // Adding the same string again, even with a different Id type, should return - // the same id. - EXPECT_THAT(value_stores.string_literal_values().Add(a).index, - Eq(a_id.index)); - EXPECT_THAT(value_stores.identifiers().Add(b).index, Eq(b_id.index)); + EXPECT_THAT(value_stores.string_literal_values().Lookup(a), Eq(a_id)); + EXPECT_THAT(value_stores.string_literal_values().Lookup("c"), + Eq(StringLiteralValueId::Invalid)); } auto MatchSharedValues(testing::Matcher ints, testing::Matcher reals, + testing::Matcher identifiers, testing::Matcher strings) -> auto { return Yaml::IsYaml(Yaml::Sequence(ElementsAre(Yaml::Mapping(ElementsAre(Pair( "shared_values", Yaml::Mapping(ElementsAre(Pair("ints", Yaml::Mapping(ints)), Pair("reals", Yaml::Mapping(reals)), + Pair("identifiers", Yaml::Mapping(identifiers)), Pair("strings", Yaml::Mapping(strings)))))))))); } @@ -113,7 +134,7 @@ TEST(ValueStore, PrintEmpty) { TestRawOstream out; value_stores.Print(out); EXPECT_THAT(Yaml::Value::FromText(out.TakeStr()), - MatchSharedValues(IsEmpty(), IsEmpty(), IsEmpty())); + MatchSharedValues(IsEmpty(), IsEmpty(), IsEmpty(), IsEmpty())); } TEST(ValueStore, PrintVals) { @@ -122,15 +143,17 @@ TEST(ValueStore, PrintVals) { value_stores.ints().Add(apint); value_stores.reals().Add( Real{.mantissa = apint, .exponent = apint, .is_decimal = true}); + value_stores.identifiers().Add("a"); value_stores.string_literal_values().Add("foo'\"baz"); TestRawOstream out; value_stores.Print(out); - EXPECT_THAT( - Yaml::Value::FromText(out.TakeStr()), - MatchSharedValues(ElementsAre(Pair("int0", Yaml::Scalar("8"))), - ElementsAre(Pair("real0", Yaml::Scalar("8*10^8"))), - ElementsAre(Pair("str0", Yaml::Scalar("foo'\"baz"))))); + EXPECT_THAT(Yaml::Value::FromText(out.TakeStr()), + MatchSharedValues( + ElementsAre(Pair("int0", Yaml::Scalar("8"))), + ElementsAre(Pair("real0", Yaml::Scalar("8*10^8"))), + ElementsAre(Pair("identifier0", Yaml::Scalar("a"))), + ElementsAre(Pair("string0", Yaml::Scalar("foo'\"baz"))))); } } // namespace diff --git a/toolchain/check/context.h b/toolchain/check/context.h index 2830503d3f91..16db3023c5e1 100644 --- a/toolchain/check/context.h +++ b/toolchain/check/context.h @@ -374,13 +374,13 @@ class Context { // Directly expose SemIR::File data accessors for brevity in calls. - auto identifiers() -> StringStoreWrapper& { + auto identifiers() -> CanonicalValueStore& { return sem_ir().identifiers(); } auto ints() -> CanonicalValueStore& { return sem_ir().ints(); } auto reals() -> ValueStore& { return sem_ir().reals(); } auto floats() -> FloatValueStore& { return sem_ir().floats(); } - auto string_literal_values() -> StringStoreWrapper& { + auto string_literal_values() -> CanonicalValueStore& { return sem_ir().string_literal_values(); } auto bind_names() -> SemIR::BindNameStore& { return sem_ir().bind_names(); } diff --git a/toolchain/check/lexical_lookup.h b/toolchain/check/lexical_lookup.h index 2c92e714bc6c..60ce5a94e8d6 100644 --- a/toolchain/check/lexical_lookup.h +++ b/toolchain/check/lexical_lookup.h @@ -37,7 +37,7 @@ class LexicalLookup { SemIR::InstId inst_id; }; - explicit LexicalLookup(const StringStoreWrapper& identifiers) + explicit LexicalLookup(const CanonicalValueStore& identifiers) : lookup_(identifiers.size() + SemIR::NameId::NonIndexValueCount) {} // Returns the lexical lookup results for a name. diff --git a/toolchain/check/scope_stack.h b/toolchain/check/scope_stack.h index 07337fd9044c..03152e72b9c8 100644 --- a/toolchain/check/scope_stack.h +++ b/toolchain/check/scope_stack.h @@ -18,7 +18,7 @@ namespace Carbon::Check { // checking within. class ScopeStack { public: - explicit ScopeStack(const StringStoreWrapper& identifiers) + explicit ScopeStack(const CanonicalValueStore& identifiers) : lexical_lookup_(identifiers) {} // A scope in which `break` and `continue` can be used. diff --git a/toolchain/driver/testdata/dump_shared_values.carbon b/toolchain/driver/testdata/dump_shared_values.carbon index 14106f23db71..b2da163d9de3 100644 --- a/toolchain/driver/testdata/dump_shared_values.carbon +++ b/toolchain/driver/testdata/dump_shared_values.carbon @@ -31,14 +31,15 @@ var str2: String = "ab'\"c"; // CHECK:STDOUT: real0: 10*10^-1 // CHECK:STDOUT: real1: 8*10^7 // CHECK:STDOUT: real2: 8*10^8 +// CHECK:STDOUT: identifiers: +// CHECK:STDOUT: identifier0: int1 +// CHECK:STDOUT: identifier1: int2 +// CHECK:STDOUT: identifier2: real1 +// CHECK:STDOUT: identifier3: real2 +// CHECK:STDOUT: identifier4: real3 +// CHECK:STDOUT: identifier5: str1 +// CHECK:STDOUT: identifier6: str2 // CHECK:STDOUT: strings: -// CHECK:STDOUT: str0: int1 -// CHECK:STDOUT: str1: int2 -// CHECK:STDOUT: str2: real1 -// CHECK:STDOUT: str3: real2 -// CHECK:STDOUT: str4: real3 -// CHECK:STDOUT: str5: str1 -// CHECK:STDOUT: str6: abc -// CHECK:STDOUT: str7: str2 -// CHECK:STDOUT: str8: 'ab''"c' +// CHECK:STDOUT: string0: abc +// CHECK:STDOUT: string1: ab'"c // CHECK:STDOUT: ... diff --git a/toolchain/sem_ir/file.h b/toolchain/sem_ir/file.h index b9899b226320..c0c017635873 100644 --- a/toolchain/sem_ir/file.h +++ b/toolchain/sem_ir/file.h @@ -91,10 +91,10 @@ class File : public Printable { auto check_ir_id() const -> CheckIRId { return check_ir_id_; } // Directly expose SharedValueStores members. - auto identifiers() -> StringStoreWrapper& { + auto identifiers() -> CanonicalValueStore& { return value_stores_->identifiers(); } - auto identifiers() const -> const StringStoreWrapper& { + auto identifiers() const -> const CanonicalValueStore& { return value_stores_->identifiers(); } auto ints() -> CanonicalValueStore& { return value_stores_->ints(); } @@ -109,11 +109,11 @@ class File : public Printable { auto floats() const -> const FloatValueStore& { return value_stores_->floats(); } - auto string_literal_values() -> StringStoreWrapper& { + auto string_literal_values() -> CanonicalValueStore& { return value_stores_->string_literal_values(); } auto string_literal_values() const - -> const StringStoreWrapper& { + -> const CanonicalValueStore& { return value_stores_->string_literal_values(); } diff --git a/toolchain/sem_ir/name.h b/toolchain/sem_ir/name.h index 3a34aeb3c09b..e83912c0b64f 100644 --- a/toolchain/sem_ir/name.h +++ b/toolchain/sem_ir/name.h @@ -23,7 +23,8 @@ namespace Carbon::SemIR { // currently a wrapper around an identifier store that has no state of its own. class NameStoreWrapper { public: - explicit NameStoreWrapper(const StringStoreWrapper* identifiers) + explicit NameStoreWrapper( + const CanonicalValueStore* identifiers) : identifiers_(identifiers) {} // Returns the requested name as a string, if it is an identifier name. This @@ -48,7 +49,7 @@ class NameStoreWrapper { auto GetIRBaseName(NameId name_id) const -> llvm::StringRef; private: - const StringStoreWrapper* identifiers_; + const CanonicalValueStore* identifiers_; }; } // namespace Carbon::SemIR