[PATCH] D77038: Fix bug in BitVector and SmallBitVector DenseMap hashing

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added a comment.

Thanks for fixing this!



================
Comment at: llvm/include/llvm/ADT/SmallBitVector.h:671
 
-  ArrayRef<uintptr_t> getData() const {
-    return isSmall() ? makeArrayRef(X) : getPointer()->getData();
+  unsigned getDenseMapHashValue() const {
+    using HashType = std::pair<size_type, ArrayRef<uintptr_t>>;
----------------
I didn't want to be too intrusive when I did it. Do you think we could do:
```
ArrayRef<uintptr_t> getData(uintptr_t &Store) const {
  if (!isSmall())
    return getPointer()->getData();
  Store = getSmallBits();
  return makeArrayRef(Store);
}
```


================
Comment at: llvm/include/llvm/ADT/SmallBitVector.h:721
-    return DenseMapInfo<std::pair<unsigned, ArrayRef<uintptr_t>>>::getHashValue(
-        std::make_pair(V.size(), V.getData()));
   }
----------------
..and only do `V.getData(Store)` here with a definition above.


================
Comment at: llvm/unittests/ADT/BitVectorTest.cpp:1234
+/// Test that capacity doesn't affect hashing.
+TYPED_TEST(BitVectorTest, DenseMapHashing) {
+  using DMI = DenseMapInfo<TypeParam>;
----------------
Can you possibly add a non-TYPED_TEST to compare a BitVector and a SmallBitVector, to ensure hashes are the same for a given content?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77038/new/

https://reviews.llvm.org/D77038