[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 09:33:24 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.

Not sure the % changes in the stage2 builds are large enough to be a significant worry...

But more importantly, while the XOR mixing looks good with a small test (compiling Clang itself), that doesn't make it robust when used with much broader inputs. FWIW, when working on hashtables, it is easy to have benchmarks and test cases that show simple solutions are faster that then struggle when the wrong input shows up, and I suspect that's how the original use of multiply ran into issues.

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


More information about the llvm-commits mailing list