[PATCH] D32509: Replace HashString algorithm with xxHash64

Vedant Kumar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 25 16:30:25 PDT 2017


vsk added a subscriber: aprantl.
vsk added a comment.

In https://reviews.llvm.org/D32509#737490, @scott.smith wrote:

> 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.


Right, it looks like that's the only thing that's changed. My worry is that if we need to change the hash again, we'd have to go through another batch of test updates. It might be easier to convince people to not print things out in an order determined by a hash function now.



================
Comment at: include/llvm/ADT/StringExtras.h:130
+{
+  return xxHash64(Str, Result);
 }
----------------
scott.smith wrote:
> 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.
Ok. I think it's fine to switch to xxh64 for now.


================
Comment at: test/DebugInfo/X86/gnu-public-names.ll:218
+; CHECK-NEXT:  [[ANON_INNER]] EXTERNAL TYPE "(anonymous namespace)::inner"
+; CHECK-NEXT:  EXTERNAL FUNCTION "f3"
 ; GCC Doesn't put local statics in pubnames, but it seems not unreasonable and
----------------
I'm not familiar with this test, but it's a bit concerning that more checks get added than removed. @aprantl, what are your thoughts on going forward with the test updates vs. making the output of llvm-dwarfdump not depend on the string hash function?


Repository:
  rL LLVM

https://reviews.llvm.org/D32509





More information about the llvm-commits mailing list