[PATCH] D77038: Fix bug in BitVector and SmallBitVector DenseMap hashing
Alexandre Ganea via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 16 12:50:14 PDT 2020
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
More information about the llvm-commits
mailing list