[llvm] [ADT] Update hash function of uint64_t for DenseMap (PR #95734)
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Sat Jun 22 12:28:06 PDT 2024
MaskRay wrote:
> > If you want to improve the hash functions used in LLVM, that seems like an interesting project. Again, I posted up thread a link to a _very_ good hash function I have been developing based on experience with Abseil's, and it works very well with `DenseMap` as well as other hashtables.
> > But I don't understand why the goal isn't to use the LLVM hashing library that is currently in the codebase, and if desired, add improvements to it. That seems better than slowly re-creating another hashing library here. =[ The original intent of the ADT/Hashing.h code was to provide a good replacement for DenseMap and other usages. If it needs to be improved to do that, by all means.
>
> +1 to this
>
> > Waiting for Hashing.h and DenseMap improvement would take too long.
>
> @MaskRay what do you mean by this? Wouldn't it be a matter of applying these changes to Hashing.h instead of here?
Current bit mixers in `Hashing.h` are too expensive for `DenseMapInfo<unsigned long>`.
We need a simpler mixer that just combines low and high bits from the input. MurmurHash-like strong mixers are overkill due to higher latency.
We've switched to `densemap::detail::mix` for 64-bit integer hashing which is simpler and avoids including `Hashing.h` in `DenseMapInfo.h`.
---
Initially, I thought changing `Hashing.h` would be complex due to Hylum's Law: many clients relied incorrectly on the iteration order of `DenseMap<StringRef, A>` or the deterministic behavior of `hash_value(StringRef)`, while `Hashing.h` is designed to be non-deterministic.
I have now fixed these clients (10+ commits) and proposed #96282 for improvement.
Should we include `Hashing.h` in `DenseMapInfo.h` eventually? Likely, but it involves more work than it seems.
https://github.com/llvm/llvm-project/pull/95734
More information about the llvm-commits
mailing list