diff --git a/.codespell_ignore b/.codespell_ignore index a47281918894..074d29fcbc5f 100644 --- a/.codespell_ignore +++ b/.codespell_ignore @@ -15,4 +15,5 @@ groupt inout parameteras pullrequest +rightt statics diff --git a/common/BUILD b/common/BUILD index effab99786f3..799a936ac26f 100644 --- a/common/BUILD +++ b/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", ], ) diff --git a/common/hashtable_key_context.h b/common/hashtable_key_context.h index 4fb3a28dc314..c38ae84af724 100644 --- a/common/hashtable_key_context.h +++ b/common/hashtable_key_context.h @@ -5,10 +5,38 @@ #ifndef CARBON_COMMON_HASHTABLE_KEY_CONTEXT_H_ #define CARBON_COMMON_HASHTABLE_KEY_CONTEXT_H_ +#include + #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 +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,17 +97,139 @@ namespace Carbon { // }; // ``` struct DefaultKeyContext { - template - auto HashKey(const KeyT& key, uint64_t seed) const -> HashCode { - return HashValue(key, seed); + template + auto HashKey(const AnyKeyT& key, uint64_t seed) const -> HashCode; + + template + 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 +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 + 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, + "No `TranslateKey` overload was provided by the derived type!"); } - template - auto KeyEq(const LHSKeyT& lhs_key, const RHSKeyT& rhs_key) const -> bool { - return lhs_key == rhs_key; - } + template + auto HashKey(const AnyKeyT& key, uint64_t seed) const -> HashCode; + + template + 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 +inline auto CarbonHashtableEq(const LeftT& lhs, const RightT& rhs) -> bool + requires(requires { + { lhs == rhs } -> std::convertible_to; + }) +{ + return lhs == rhs; +} + +template +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 +inline auto HashtableEq(const LeftT& lhs, const RightT& rhs) -> bool { + return InternalHashtableEqDispatch::DispatchImpl(lhs, rhs); +} + +template +auto DefaultKeyContext::HashKey(const AnyKeyT& key, uint64_t seed) const + -> HashCode { + return HashValue(key, seed); +} + +template +auto DefaultKeyContext::KeyEq(const AnyKeyT& lhs_key, + const TableKeyT& rhs_key) const -> bool { + return HashtableEq(lhs_key, rhs_key); +} + +template +template +auto TranslatingKeyContext::HashKey(const AnyKeyT& key, + uint64_t seed) const -> HashCode { + const DerivedT& self = *static_cast(this); + if constexpr (requires { self.TranslateKey(key); }) { + return HashValue(self.TranslateKey(key), seed); + } else { + return HashValue(key, seed); + } +} + +template +template +auto TranslatingKeyContext::KeyEq(const AnyKeyT& lhs_key, + const TableKeyT& rhs_key) const + -> bool { + const DerivedT& self = *static_cast(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 #endif // CARBON_COMMON_HASHTABLE_KEY_CONTEXT_H_ diff --git a/common/hashtable_key_context_test.cpp b/common/hashtable_key_context_test.cpp new file mode 100644 index 000000000000..19c31627e0e2 --- /dev/null +++ b/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 +#include + +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 { + auto TranslateKey(int index) const -> const llvm::APInt& { + return array[index]; + } + + llvm::ArrayRef 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 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 diff --git a/common/map.h b/common/map.h index 232df5f83f40..9cc92561282f 100644 --- a/common/map.h +++ b/common/map.h @@ -224,7 +224,7 @@ class MapBase : protected RawHashtable::BaseImpl - void ForEach(CallbackT callback) + void ForEach(CallbackT callback) const requires(std::invocable) { return ViewT(*this).ForEach(callback); diff --git a/common/raw_hashtable.h b/common/raw_hashtable.h index 8adff07b5928..ff5fdace03c1 100644 --- a/common/raw_hashtable.h +++ b/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 +template 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 +template class ViewImpl { protected: using KeyT = InputKeyT; diff --git a/common/raw_hashtable_test_helpers.h b/common/raw_hashtable_test_helpers.h index b304fd339ea8..83d8bf58e3ab 100644 --- a/common/raw_hashtable_test_helpers.h +++ b/common/raw_hashtable_test_helpers.h @@ -86,20 +86,17 @@ struct FixedHashKeyContext : DefaultKeyContext { }; template -class IndexKeyContext { +class IndexKeyContext : public TranslatingKeyContext> { + using Base = TranslatingKeyContext; + public: explicit IndexKeyContext(llvm::ArrayRef 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. diff --git a/toolchain/sem_ir/generic.cpp b/toolchain/sem_ir/generic.cpp index b3a66f8ecad9..dc121010d630 100644 --- a/toolchain/sem_ir/generic.cpp +++ b/toolchain/sem_ir/generic.cpp @@ -6,7 +6,8 @@ namespace Carbon::SemIR { -class GenericInstanceStore::KeyContext { +class GenericInstanceStore::KeyContext + : public TranslatingKeyContext { public: // A lookup key for a generic instance. struct Key { @@ -19,21 +20,10 @@ class GenericInstanceStore::KeyContext { explicit KeyContext(llvm::ArrayRef 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 - auto HashKey(KeyT key, uint64_t seed) const -> HashCode { - return HashValue(AsKey(key), seed); - } - - template - auto KeyEq(const LHSKeyT& lhs_key, const RHSKeyT& rhs_key) const -> bool { - return AsKey(lhs_key) == AsKey(rhs_key); - } private: llvm::ArrayRef instances_;