[PATCH] D84782: [PGO] Include the mem ops into the function hash.

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

Rong has pointed out that having 2 bits may not be enough -- also instead of using | operator,  add is better.

Example:  for a comdat function with two copies: one has 4 memop sites, while the other has zero. Assuming both has one icall site and cfg and selects are the same, then their resulting hash will still clash if Or operator is used.

Another example, similar to the above, but one has 5 sites, the other has 1 sites -- the resulting hash is also the same. Even though both copies will have vp data statically allocated, we may end up with out of bound access if the inline instance that assumes 5 sites is using the profv with only 1 site statically allocated.

I think we should at least change the '|' to +.

Better yet, just use JamRC to compute the higher 32bit hash using the four values.



================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:642
 
   // Hash format for context sensitive profile. Reserve 4 bits for other
   // information.
----------------
The comment looks stale.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84782