[PATCH] D113688: [CSSPGO] Fix a hash code truncating issue in ContextTrieNode.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:183-185
+  uint64_t NameHash = std::hash<std::string>{}(ChildName.str());
+  uint64_t LocId = (Callsite.LineOffset << 16) | Callsite.Discriminator;
   return NameHash + (LocId << 5) + LocId;
----------------
wenlei wrote:
> This `nodeHash` is the blend function similar to DJB and it's assuming 32-bit length, see the shift 16. If we are fully switching to 64 bit hash, we should change the blend function too.
> 
> However, does the collision come from the blend function (`nodeHash`), or lower part of `std::hash`? I guess it's the later, then I'm curious what strings actually lead to collision on the lower 32 bit? 
> 
> 
Good point! Is `(Callsite.LineOffset << 32) | Callsite.Discriminator;` a good form for 64-bit? 

Yes, the collision came from a colliding lower-32-bit of std::hash


```
Same lower 32-bit: 2086825821
Name1:          _ZNSt8__detail15_Hash_code_baseINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEESt4pairIKS6_PmENS_10_Select1stESt4hashIS6_ENS_18_Mod_range_hashingENS_20_Default_ranged_hashELb1EE10_M_extractEv
                  _
Name2: ZSt12__niter_baseIPN4onnx3UseESt6vectorIS1_SaIS1_EEET_N9__gnu_cxx17__normal_iteratorIS6_T0_EE
```






Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113688/new/

https://reviews.llvm.org/D113688