[BOLT][NFC] Refactor function matching

[shawbyoung]

Moved function matching techniques into separate helper functions for
ease of understanding and to make space for additional function
matching techniques to be added (e.g. call graph function matching).

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Shaw Young (shawbyoung)

Changes

Moved function matching techniques into separate helper functions.

---

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

2 Files Affected:

• (modified) bolt/include/bolt/Profile/YAMLProfileReader.h (+13)
• (modified) bolt/lib/Profile/YAMLProfileReader.cpp (+49-38)

diff --git a/bolt/include/bolt/Profile/YAMLProfileReader.h b/bolt/include/bolt/Profile/YAMLProfileReader.h
index 7a8aa176c30f1..a5bd3544bd999 100644
--- a/bolt/include/bolt/Profile/YAMLProfileReader.h
+++ b/bolt/include/bolt/Profile/YAMLProfileReader.h
@@ -73,6 +73,10 @@ class YAMLProfileReader : public ProfileReaderBase {
   bool parseFunctionProfile(BinaryFunction &Function,
                             const yaml::bolt::BinaryFunctionProfile &YamlBF);
 
+  /// Returns block cnt equality if IgnoreHash is true, otherwise, hash equality
+  bool profileMatches(const yaml::bolt::BinaryFunctionProfile &Profile,
+                      BinaryFunction &BF);
+
   /// Infer function profile from stale data (collected on older binaries).
   bool inferStaleProfile(BinaryFunction &Function,
                          const yaml::bolt::BinaryFunctionProfile &YamlBF);
@@ -80,6 +84,15 @@ class YAMLProfileReader : public ProfileReaderBase {
   /// Initialize maps for profile matching.
   void buildNameMaps(BinaryContext &BC);
 
+  /// Matches functions using exact name.
+  size_t matchWithExactName();
+
+  /// Matches function using LTO comomon name.
+  size_t matchWithLTOCommonName();
+
+  /// Matches functions using exact hash.
+  size_t matchWithHash(BinaryContext &BC);
+
   /// Update matched YAML -> BinaryFunction pair.
   void matchProfileToFunction(yaml::bolt::BinaryFunctionProfile &YamlBF,
                               BinaryFunction &BF) {
diff --git a/bolt/lib/Profile/YAMLProfileReader.cpp b/bolt/lib/Profile/YAMLProfileReader.cpp
index 554def697fa21..e8ce187367899 100644
--- a/bolt/lib/Profile/YAMLProfileReader.cpp
+++ b/bolt/lib/Profile/YAMLProfileReader.cpp
@@ -333,6 +333,12 @@ Error YAMLProfileReader::preprocessProfile(BinaryContext &BC) {
 
   return Error::success();
 }
+bool YAMLProfileReader::profileMatches(
+    const yaml::bolt::BinaryFunctionProfile &Profile, BinaryFunction &BF) {
+  if (opts::IgnoreHash)
+    return Profile.NumBasicBlocks == BF.size();
+  return Profile.Hash == static_cast<uint64_t>(BF.getHash());
+}
 
 bool YAMLProfileReader::mayHaveProfileData(const BinaryFunction &BF) {
   if (opts::MatchProfileWithFunctionHash)
@@ -350,44 +356,8 @@ bool YAMLProfileReader::mayHaveProfileData(const BinaryFunction &BF) {
   return false;
 }
 
-Error YAMLProfileReader::readProfile(BinaryContext &BC) {
-  if (opts::Verbosity >= 1) {
-    outs() << "BOLT-INFO: YAML profile with hash: ";
-    switch (YamlBP.Header.HashFunction) {
-    case HashFunction::StdHash:
-      outs() << "std::hash\n";
-      break;
-    case HashFunction::XXH3:
-      outs() << "xxh3\n";
-      break;
-    }
-  }
-  YamlProfileToFunction.resize(YamlBP.Functions.size() + 1);
-
-  auto profileMatches = [](const yaml::bolt::BinaryFunctionProfile &Profile,
-                           BinaryFunction &BF) {
-    if (opts::IgnoreHash)
-      return Profile.NumBasicBlocks == BF.size();
-    return Profile.Hash == static_cast<uint64_t>(BF.getHash());
-  };
-
-  uint64_t MatchedWithExactName = 0;
-  uint64_t MatchedWithHash = 0;
-  uint64_t MatchedWithLTOCommonName = 0;
-
-  // Computes hash for binary functions.
-  if (opts::MatchProfileWithFunctionHash) {
-    for (auto &[_, BF] : BC.getBinaryFunctions()) {
-      BF.computeHash(YamlBP.Header.IsDFSOrder, YamlBP.Header.HashFunction);
-    }
-  } else if (!opts::IgnoreHash) {
-    for (BinaryFunction *BF : ProfileBFs) {
-      if (!BF)
-        continue;
-      BF->computeHash(YamlBP.Header.IsDFSOrder, YamlBP.Header.HashFunction);
-    }
-  }
-
+size_t YAMLProfileReader::matchWithExactName() {
+  size_t MatchedWithExactName = 0;
   // This first pass assigns profiles that match 100% by name and by hash.
   for (auto [YamlBF, BF] : llvm::zip_equal(YamlBP.Functions, ProfileBFs)) {
     if (!BF)
@@ -402,10 +372,14 @@ Error YAMLProfileReader::readProfile(BinaryContext &BC) {
       ++MatchedWithExactName;
     }
   }
+  return MatchedWithExactName;
+}
 
+size_t YAMLProfileReader::matchWithHash(BinaryContext &BC) {
   // Iterates through profiled functions to match the first binary function with
   // the same exact hash. Serves to match identical, renamed functions.
   // Collisions are possible where multiple functions share the same exact hash.
+  size_t MatchedWithHash = 0;
   if (opts::MatchProfileWithFunctionHash) {
     DenseMap<size_t, BinaryFunction *> StrictHashToBF;
     StrictHashToBF.reserve(BC.getBinaryFunctions().size());
@@ -424,8 +398,12 @@ Error YAMLProfileReader::readProfile(BinaryContext &BC) {
       }
     }
   }
+  return MatchedWithHash;
+}
 
+size_t YAMLProfileReader::matchWithLTOCommonName() {
   // This second pass allows name ambiguity for LTO private functions.
+  size_t MatchedWithLTOCommonName = 0;
   for (const auto &[CommonName, LTOProfiles] : LTOCommonNameMap) {
     if (!LTOCommonNameFunctionMap.contains(CommonName))
       continue;
@@ -456,6 +434,39 @@ Error YAMLProfileReader::readProfile(BinaryContext &BC) {
       ++MatchedWithLTOCommonName;
     }
   }
+  return MatchedWithLTOCommonName;
+}
+
+Error YAMLProfileReader::readProfile(BinaryContext &BC) {
+  if (opts::Verbosity >= 1) {
+    outs() << "BOLT-INFO: YAML profile with hash: ";
+    switch (YamlBP.Header.HashFunction) {
+    case HashFunction::StdHash:
+      outs() << "std::hash\n";
+      break;
+    case HashFunction::XXH3:
+      outs() << "xxh3\n";
+      break;
+    }
+  }
+  YamlProfileToFunction.resize(YamlBP.Functions.size() + 1);
+
+  // Computes hash for binary functions.
+  if (opts::MatchProfileWithFunctionHash) {
+    for (auto &[_, BF] : BC.getBinaryFunctions()) {
+      BF.computeHash(YamlBP.Header.IsDFSOrder, YamlBP.Header.HashFunction);
+    }
+  } else if (!opts::IgnoreHash) {
+    for (BinaryFunction *BF : ProfileBFs) {
+      if (!BF)
+        continue;
+      BF->computeHash(YamlBP.Header.IsDFSOrder, YamlBP.Header.HashFunction);
+    }
+  }
+
+  size_t MatchedWithExactName = matchWithExactName();
+  size_t MatchedWithHash = matchWithHash(BC);
+  size_t MatchedWithLTOCommonName = matchWithLTOCommonName();
 
   for (auto [YamlBF, BF] : llvm::zip_equal(YamlBP.Functions, ProfileBFs))
     if (!YamlBF.Used && BF && !ProfiledFunctions.count(BF))

[dcci]

I have a couple of general comments about this.
Can you also please add a description explaining what this patch does?

[dcci]

I have a couple of general comments about this. Can you also please add a description explaining what this patch does?

i.e. why we're refactoring these functions.

[aaupov]

LG % nit