[PATCH] D32509: Replace HashString algorithm with xxHash64

[Scott Smith via Phabricator via llvm-commits]

scott.smith added a comment.

In https://reviews.llvm.org/D32509#737488, @vsk wrote:

> Thanks for working on this! In the future, please set llvm-commits as a subscriber on differential revisions, not as a reviewer. Also, please upload patches with context (e.g "git diff -U1000").


The list is both a subscriber and a reviewer.  Next time I'll only make it a subscriber.
I assumed I had no context because Phabricator couldn't map my patch to the repository.  Your comment makes me think it doesn't even try (ReviewBoard does this automatically).  I didn't realize I'm supposed to supply the context!

> IMO, tests which break when the string hash function is changed are suspicious. Why is a dependence on a specific hash function being tested? It might make sense to fix the tests in prep commits (ideally no test updates would be needed with this patch). I'll look into fixing the llvm-{cov,profdata} issues.

That was my first thought too - I was worried the hash value was encoded in the output some how.  But I think this stems from the use of StringMap and StringSet, which place into buckets by hash value.  So it isn't that anything interesting changed, just that the order of iteration changed.  None of the interesting contents changed, AFAICT.



================
Comment at: include/llvm/ADT/StringExtras.h:130
+{
+  return xxHash64(Str, Result);
 }
----------------
vsk wrote:
> Would xxh32 run significantly faster/slower than xxh64?
It isn't already in the llvm code base.  If we're going that route of adding a hash function, there are probably even better choices.


Repository:
  rL LLVM

https://reviews.llvm.org/D32509