[llvm] [ADT] Update hash function of uint64_t for DenseMap (PR #95734)

Chandler Carruth via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 18 16:37:18 PDT 2024


chandlerc wrote:

> > Compile-time for the PR as proposed: http://llvm-compile-time-tracker.com/compare.php?from=3ca17443ef4af21bdb1f3b4fbcfff672cbc6176c&to=9cd0b838265006ff699153bfbb1a1a39ebfb9cdd&stat=instructions:u
> > Compile-time using xor mixing (that is, [820edb5](https://github.com/llvm/llvm-project/commit/820edb52ce1abc090f41c5210dcb04edb6203f36) applied on top): http://llvm-compile-time-tracker.com/compare.php?from=3ca17443ef4af21bdb1f3b4fbcfff672cbc6176c&to=820edb52ce1abc090f41c5210dcb04edb6203f36&stat=instructions%3Au
> > It doesn't seem like we get any benefit out of the better mixing using combineHashValue() for average-case compilation, so I think you should stick to your first variant using xor.
> 
> People use metaheuristics to find good bit mixer functions that achieve good ratings on some metrics (e.g. PractRand) [jonkagstrom.com/bit-mixer-construction](https://jonkagstrom.com/bit-mixer-construction/)
> 
> `combineHashValue` is a custom bit mixer from 2008 ([5fc8ab6](https://github.com/llvm/llvm-project/commit/5fc8ab6d187aefbf1d2cbd36e191e675b14db8f6)) that is probably not good. I propose to change it in #95970
> 
> This patch can still use `combineHashValue` , but probably change `combineHashValue((uint32_t)x, x>>32)` to `combineHashValue(x>>32, (uint32_t)x)` to avoid a ROR operation.

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.

https://github.com/llvm/llvm-project/pull/95734


More information about the llvm-commits mailing list