[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