[llvm] [Support] Fix VersionTuple DenseMapInfo conformance (PR #206872)

[via llvm-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-support

Author: Daniel Rodríguez Troitiño (drodriguez)

<details>
<summary>Changes</summary>

In C++ standard library (and many other programming languages standard libraries), if two values are equal, they should return the same hash. This requirement is pretty common so associative containers can quickly find values that might be equal by calculating the hash, and if that requirement is not hold, associative containers might not work as expected.

The documentation of `DenseMapInfo` does not specify the same requirement, as far as I can see, but its usage on `DenseMap` relies on it, or objects that compare equal might end up in different buckets and will not be correctly found.

`DenseMapInfo<VersionTuple>` implementation of `getHashValue` was implementing its own logic for hashing, but delegating to `VersionTuple` for equality. `VersionTuple` equality partially compares its scalar member variables, skipping some boolean member variables, but the `getHashValue` implementation was using those boolean member variables as well, creating different hashes for `VersionTuple` that were considered equal. This divergence was creating problems when `VersionTuple` was used as keys for `DenseMap` in some cases.

`VersionTuple` also implements LLVM Hashing `hash_value`, and the implementation of that matches the equality operator, so switch `getHashValue` to delegate to `hash_value`, the same that `isEqual` delegates to the equality operator to keep the logic synchronized.

Includes a small unit test that verifies that when `isEqual` returns true, the return values of `getHashValue` are the same. This test failed before these changes.

---
Full diff: https://github.com/llvm/llvm-project/pull/206872.diff


2 Files Affected:

- (modified) llvm/include/llvm/Support/VersionTuple.h (+1-11) 
- (modified) llvm/unittests/Support/VersionTupleTest.cpp (+19) 


``````````diff
diff --git a/llvm/include/llvm/Support/VersionTuple.h b/llvm/include/llvm/Support/VersionTuple.h
index cae3a88e7a429..c661fd003d0d0 100644
--- a/llvm/include/llvm/Support/VersionTuple.h
+++ b/llvm/include/llvm/Support/VersionTuple.h
@@ -216,17 +216,7 @@ LLVM_ABI raw_ostream &operator<<(raw_ostream &Out, const VersionTuple &V);
 // Provide DenseMapInfo for version tuples.
 template <> struct DenseMapInfo<VersionTuple> {
   static unsigned getHashValue(const VersionTuple &Value) {
-    unsigned Result = Value.getMajor();
-    if (auto Minor = Value.getMinor())
-      Result = detail::combineHashValue(Result, *Minor);
-    if (auto Subminor = Value.getSubminor())
-      Result = detail::combineHashValue(Result, *Subminor);
-    if (auto Build = Value.getBuild())
-      Result = detail::combineHashValue(Result, *Build);
-    if (auto Subbuild = Value.getSubbuild())
-      Result = detail::combineHashValue(Result, *Subbuild);
-
-    return Result;
+    return hash_value(Value);
   }
 
   static bool isEqual(const VersionTuple &LHS, const VersionTuple &RHS) {
diff --git a/llvm/unittests/Support/VersionTupleTest.cpp b/llvm/unittests/Support/VersionTupleTest.cpp
index 724e365f82360..7514b4b81f7c6 100644
--- a/llvm/unittests/Support/VersionTupleTest.cpp
+++ b/llvm/unittests/Support/VersionTupleTest.cpp
@@ -99,3 +99,22 @@ TEST(VersionTuple, withMajorReplaced) {
   EXPECT_TRUE(ReplacedVersion.getSubbuild().has_value());
   EXPECT_EQ(VersionTuple(7, 11, 12, 2, 8), ReplacedVersion);
 }
+
+TEST(VersionTuple, DenseMapInfo) {
+  VersionTuple VT16(16);
+  VersionTuple VT16_0(16, 0);
+
+  VersionTuple VT17(17);
+  VersionTuple VT17_0(17, 0);
+
+  // In C++, if two objects are equal, their hashes should be equal.
+  // DenseMapInfo relies in the same relation for comparing keys.
+  // If isEqual returns true, getHashValue should return the same value.
+  EXPECT_TRUE(DenseMapInfo<VersionTuple>::isEqual(VT16, VT16_0));
+  EXPECT_EQ(DenseMapInfo<VersionTuple>::getHashValue(VT16),
+            DenseMapInfo<VersionTuple>::getHashValue(VT16_0));
+
+  EXPECT_TRUE(DenseMapInfo<VersionTuple>::isEqual(VT17, VT17_0));
+  EXPECT_EQ(DenseMapInfo<VersionTuple>::getHashValue(VT17),
+            DenseMapInfo<VersionTuple>::getHashValue(VT17_0));
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/206872