Skip to content
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.

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

[BOLT][NFC] Avoid computing BF hash twice in YAML reader #75096

Merged
merged 1 commit into from
May 24, 2024

Conversation

aaupov
Copy link
Contributor

@aaupov aaupov commented Dec 11, 2023

We compute BF hashes in YAMLProfileReader::readProfile when first
matching profile functions with binary functions, and second time in
YAMLProfileReader::parseFunctionProfile during the profile assignment
(we need to do that to account for LTO private functions with
mismatching suffix).

Avoid recomputing the hash if it's been set.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 27, 2024

@llvm/pr-subscribers-bolt

Author: Amir Ayupov (aaupov)

Changes

We compute BF hashes in YAMLProfileReader::readProfile when first
matching profile functions with binary functions, and second time in
YAMLProfileReader::parseFunctionProfile during the profile assignment
(we need to do that to account for LTO private functions with
mismatching suffix).

Avoid recomputing the hash if it's been set.


Full diff: https://github.com/llvm/llvm-project/pull/75096.diff

2 Files Affected:

  • (modified) bolt/lib/Core/BinaryFunction.cpp (+7)
  • (modified) bolt/lib/Profile/YAMLProfileReader.cpp (+8-5)
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 1fa96dfaabde81..c4d3b65788b781 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -3685,6 +3685,13 @@ BinaryFunction::BasicBlockListType BinaryFunction::dfs() const {
 
 size_t BinaryFunction::computeHash(bool UseDFS, HashFunction HashFunction,
                                    OperandHashFuncTy OperandHashFunc) const {
+  LLVM_DEBUG({
+    dbgs() << "BOLT-DEBUG: computeHash " << getPrintName() << ' '
+           << (UseDFS ? "dfs" : "bin") << " order "
+           << (HashFunction == HashFunction::StdHash ? "std::hash" : "xxh3")
+           << '\n';
+  });
+
   if (size() == 0)
     return 0;
 
diff --git a/bolt/lib/Profile/YAMLProfileReader.cpp b/bolt/lib/Profile/YAMLProfileReader.cpp
index e4673f6e3c301d..cbc2f2025e7e93 100644
--- a/bolt/lib/Profile/YAMLProfileReader.cpp
+++ b/bolt/lib/Profile/YAMLProfileReader.cpp
@@ -99,11 +99,14 @@ bool YAMLProfileReader::parseFunctionProfile(
       FuncRawBranchCount += YamlSI.Count;
   BF.setRawBranchCount(FuncRawBranchCount);
 
-  if (!opts::IgnoreHash &&
-      YamlBF.Hash != BF.computeHash(IsDFSOrder, HashFunction)) {
-    if (opts::Verbosity >= 1)
-      errs() << "BOLT-WARNING: function hash mismatch\n";
-    ProfileMatched = false;
+  if (!opts::IgnoreHash) {
+    if (!BF.getHash())
+      BF.computeHash(IsDFSOrder, HashFunction);
+    if (YamlBF.Hash != BF.getHash()) {
+      if (opts::Verbosity >= 1)
+        errs() << "BOLT-WARNING: function hash mismatch\n";
+      ProfileMatched = false;
+    }
   }
 
   if (YamlBF.NumBasicBlocks != BF.size()) {

Copy link
Contributor

@rafaelauler rafaelauler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

We compute BF hashes in `YAMLProfileReader::readProfile` when first
matching profile functions with binary functions, and second time in
`YAMLProfileReader::parseFunctionProfile` during the profile assignment
(we need to do that to account for LTO private functions with
mismatching suffix).

Avoid recomputing the hash if it's been set.
@aaupov aaupov merged commit 720cade into llvm:main May 24, 2024
6 checks passed
@aaupov aaupov deleted the revert-xxh branch May 24, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants