[llvm] [ADT] Update hash function of uint64_t for DenseMap (PR #95734)
Ilya Biryukov via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 17 06:15:42 PDT 2024
ilya-biryukov wrote:
> > ```
> > * code relying on existing order in hash tables breaks when the order changes (there shouldn't be code like this, but [Hyrum's law](https://www.hyrumslaw.com/)).
> > ```
>
> There may be some test fallout to deal with, but I think we should be in a fairly good position thanks to reverse-iteration testing.
Thanks for the pointers, I didn't know we had this.
Looking at the [implementation](https://github.com/llvm/llvm-project/blob/457e8954798b707c73ec76e0819760aaf65d0ffb/llvm/include/llvm/Support/ReverseIteration.h#L12), we only seem to do it for pointers. I expect maps with integer keys to be very frequent too and they don't seem to be covered by existing machinery for reverse iteration. Especially if we go with replacing hash functions for all integer types/all types rather than just 64bit ints, as you suggested in the comment thread.
Another potential problem is downstream uses that aren't covered by the upstream tests we have. I am sure it's manageable, but we would want to announce the change a little in advance so that people know it's coming.
> > ```
> > * The C++ standard returns 64 bit hash values (on 64 bit platforms), maybe we should do the same in LLVM? I suspect the code of our hash tables would work just fine with it (although it might add some overhead if we store precomputed hashes somewhere).
> > ```
>
> We currently only use the low bits of the hash, so extending it to 64 bits is not useful at present.
Sorry for the confusing wording on my part, I referred to not just extending the returned hash function to 64 bit, but also making sure we use all 64bits in our hash table implementation.
> LLVM is definitely behind the state of the art -- if someone finds the time to port DenseMap and SmallPtrSet to something like swiss tables, that would be great. (And not just for performance, we could also get rid of the need to specify explicit tombstone and empty keys for types.)
+1, I know that @chandlerc had some new ideas for hash tables in the works too, but I'm not sure how fleshed out they are and whether he has time to land them / describe them in enough detail that someone else can pick them up.
https://github.com/llvm/llvm-project/pull/95734
More information about the llvm-commits
mailing list