[Serialization] Use specialized decl hash function for GlobalDeclID #95730
Conversation
See the comment: llvm#92083 (comment) After the patch, llvm#92083, the lower 32 bits of DeclID can be the same commonly. This may produce many collisions. It will be best to change the default hash implementation for uint64_t. But sent this one as a quick workaround.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Chuanqi Xu (ChuanqiXu9) ChangesSee the comment: After the patch, #92083, the lower 32 bits of DeclID can be the same commonly. This may produce many collisions. It will be best to change the default hash implementation for uint64_t. But sent this one as a quick workaround. Full diff: https://github.com/llvm/llvm-project/pull/95730.diff 1 Files Affected:
diff --git a/clang/include/clang/AST/DeclID.h b/clang/include/clang/AST/DeclID.h
index 32d2ed41a374a..4ad7afb463b18 100644
--- a/clang/include/clang/AST/DeclID.h
+++ b/clang/include/clang/AST/DeclID.h
@@ -230,7 +230,11 @@ template <> struct DenseMapInfo<clang::GlobalDeclID> {
}
static unsigned getHashValue(const GlobalDeclID &Key) {
- return DenseMapInfo<DeclID>::getHashValue(Key.get());
+ // Our default hash algorithm for 64 bits integer may not be very good.
+ // In GlobalDeclID's case, it is pretty common that the lower 32 bits can
+ // be same.
+ return DenseMapInfo<uint32_t>::getHashValue(Key.getModuleFileIndex()) ^
+ DenseMapInfo<uint32_t>::getHashValue(Key.getLocalDeclIndex());
}
static bool isEqual(const GlobalDeclID &L, const GlobalDeclID &R) {
|
clang/include/clang/AST/DeclID.h
Outdated
| // Our default hash algorithm for 64 bits integer may not be very good. | ||
| // In GlobalDeclID's case, it is pretty common that the lower 32 bits can | ||
| // be same. | ||
| return DenseMapInfo<uint32_t>::getHashValue(Key.getModuleFileIndex()) ^ |
There was a problem hiding this comment.
Can you use hash_combine from llvm/ADT/Hash.h?
There was a problem hiding this comment.
+1 to using hash_combine, it should provide better results than xor.
Update: after asking around, I think hash_value from ADT/Hashing.h on the underlying int64 value would give good results here and make the code even simpler.
See the comment:
#92083 (comment)
After the patch, #92083, the lower 32 bits of DeclID can be the same commonly. This may produce many collisions. It will be best to change the default hash implementation for uint64_t. But sent this one as a quick workaround.
Feel free to update this if you prefer other hash functions.