Skip to content

[Bolt] issue about using std::hash in YamlProfileReader and its test case #65241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
linsinan1995 opened this issue Sep 4, 2023 · 5 comments · Fixed by #72542
Closed

[Bolt] issue about using std::hash in YamlProfileReader and its test case #65241

linsinan1995 opened this issue Sep 4, 2023 · 5 comments · Fixed by #72542
Assignees
@spupyrev
Labels
BOLT good first issue https://github.com/llvm/llvm-project/contribute help wanted Indicates that a maintainer wants help. Not [good first issue].

Comments

@linsinan1995
Copy link
Member

linsinan1995 commented Sep 4, 2023 •
edited
Loading

it seems that bolt tries to match block counts based on hashes generated from std::hash when the data is yaml format. However, std::hash does not guarantee the result will be the same from different setups.

I recently used a llvm-bolt linked against libc++, and it failed on test case X86/reader-stale-yaml.test, since the std::hash results are different from these two libraries...

image
@llvmbot
Copy link
Member

llvmbot commented Sep 4, 2023

@llvm/issue-subscribers-bolt

@aaupov
Copy link
Contributor

aaupov commented Sep 5, 2023

Thanks for bringing this up. We should fix this since it fails with libcxx, perhaps by switching to xxh3 just like in couple other places in llvm.

@aaupov aaupov mentioned this issue Sep 6, 2023
Closed
aaupov added a commit to aaupov/llvm-project that referenced this issue Sep 6, 2023
@aaupov
[BOLT] Switch BF::computeHash to xxh3
b9e0313 
std::hash is C++ STL implementation-specific. Switch to xxh3 as the fast and
STL-independent alternative for basic block hashes which are exposed to users
in YAML profile.

Fixes llvm#65241.
@linsinan1995
Copy link
Member Author

Thanks for bringing this up. We should fix this since it fails with libcxx, perhaps by switching to xxh3 just like in couple other places in llvm.

Hi @aaupov , it looks like a sound solution, and it would be nicer if you could also fix the other two std::hash usages in bolt/lib/Profile/StaleProfileMatching.cpp :)

@aaupov
Copy link
Contributor

aaupov commented Sep 19, 2023

Yes, but modifying stale matching hashes creates a problem that checked in tests no longer match the intended way. Leaving the issue open.

@aaupov aaupov assigned aaupov and unassigned aaupov Sep 19, 2023
@aaupov aaupov added help wanted Indicates that a maintainer wants help. Not [good first issue]. good first issue https://github.com/llvm/llvm-project/contribute labels Sep 19, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 19, 2023

@llvm/issue-subscribers-good-first-issue

it seems that bolt tries to match block counts based on hashes generated from std::hash when the data is yaml format. However, std::hash does not guarantee the result will be the same from different setups.

I recently used a llvm-bolt linked against libc++, and it failed on test case X86/reader-stale-yaml.test, since the std::hash results are different from these two libraries...

<img width="505" alt="image" src="https://github.com/llvm/llvm-project/assets/47880367/6e7dd792-37ba-48d7-80c8-6b84897f3a94">

@spupyrev spupyrev self-assigned this Nov 16, 2023
@spupyrev spupyrev mentioned this issue Nov 16, 2023
Merged
@aaupov aaupov linked a pull request Nov 27, 2023 that will close this issue
[BOLT] Use deterministic xxh3 for computing BF/BB hashes #72542
Merged
spupyrev added a commit that referenced this issue Nov 27, 2023
@spupyrev
[BOLT] Use deterministic xxh3 for computing BF/BB hashes (#72542)
e7dd596 
std::hash and ADT/Hashing::hash_value are non-deterministic functions
whose
results might vary across implementation/process/execution. Using xxh3
instead
for computing hashes of BinaryFunctions and BinaryBasicBlock for stale
profile
matching.
(A possible alternative is to use ADT/StableHashing.h based on FNV
hashing but
xxh3 seems to be more popular in LLVM)

This is to address #65241.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@spupyrev spupyrev

Labels
BOLT good first issue https://github.com/llvm/llvm-project/contribute help wanted Indicates that a maintainer wants help. Not [good first issue].
Projects
None yet
Milestone
No milestone
4 participants
@aaupov @spupyrev @linsinan1995 @llvmbot