[BOLT] Provide backwards compatibility for YAML profile with std::hash #74253
Conversation
|
aaupov
force-pushed
the
revert-xxh
branch
2 times, most recently
from
268cc19 to
c7c5bf9
Compare
There was a problem hiding this comment.
How about we write a new field to YAML with a hash function name/ID? In the reader, we default to std::hash for backwards compatibility as you already do. If we are not planning on using std::hash in the future (for writing), perhaps the option is not needed at all? We can likely have a couple of binary/YAML test cases that verify backwards compatibility.
Suggestion for the title: "Provide backwards compatibility for YAML profile with std::hash".
aaupov
changed the title
Added a YAML test checking backwards compatibility. It reuses the test |
|
Renamed Xxh3 to XXH3. |
Summary: xxh3 hash is the default for newly produced profile (sets `hash-func: xxh3`), whereas the profile that doesn't specify `hash-func` will be treated as `hash-func: std-hash`, preserving old behavior.
|
bolt/test/X86/reader-stale-yaml-std.test introduced by this patch might rely on determinism of My Hashing.h change #96282 might cause bolt/test/X86/reader-stale-yaml-std.test to fail: #96282 (comment) |
| BlendedHashes[I].InstrHash = (uint16_t)InstrHash; | ||
| if (HashFunction == HashFunction::StdHash) { | ||
| uint64_t InstrHash = std::hash<std::string>{}(InstrHashStr); | ||
| BlendedHashes[I].InstrHash = (uint16_t)hash_value(InstrHash); |
There was a problem hiding this comment.
std::hash<std::string> results are quite good and do not need another bit mixer.
llvm::hash_value is non-deterministic (#96282).
There was a problem hiding this comment.
Yes, we realized that but couldn't make the change at the time (breaks profile matching). But now that the default has switched to xxh3 we can drop hash_value and adjust tests.
This test from llvm#74253 relies on particular hash values from Hashing.h. The test fails in LLVM_ENABLE_ABI_BREAKING_CHECKS=on modes (llvm#96282) or whenever Hashing.h implementation changes.
This test from llvm#74253 relies on particular hash values from Hashing.h. The test fails in LLVM_ENABLE_ABI_BREAKING_CHECKS=on modes (llvm#96282) or whenever Hashing.h implementation changes.
Provide backwards compatibility for YAML profile that uses
std::hash:xxh3 hash is the default for newly produced profile (sets
std-hash: false),whereas the profile that doesn't specify
std-hashwill be treated asstd-hash: true, preserving old behavior.Same as
profile-use-dfsin https://reviews.llvm.org/D156176.