[PATCH] D144807: [BOLT] Change call count output for ICF

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb created this revision.
maksfb added reviewers: yota9, Amir, ayermolo, rafauler, yavtuk, treapster.
Herald added a project: All.
maksfb requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

ICF optimization runs multiple passes and the order in which functions
are folded could be dependent on the order they are being processed.
This order is indeterministic as functions are intermediately stored in
std::unordered_map<>. Note that this order is mostly stable, but is not
guaranteed to be and can change e.g. after switching to a different C++
library implementation.

Because the processing (and folding) order is indeterministic, the
previous way of calculating merged function call count could produce
different results.

Change the way we calculate the ICF call count to make it independent of
the function folding/processing order.

Mostly NFC as the output binary should remain the same, the change
affects only the console output.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144807

Files:
  bolt/include/bolt/Core/BinaryFunction.h
  bolt/lib/Core/BinaryContext.cpp
  bolt/lib/Passes/IdenticalCodeFolding.cpp


Index: bolt/lib/Passes/IdenticalCodeFolding.cpp
===================================================================
--- bolt/lib/Passes/IdenticalCodeFolding.cpp
+++ bolt/lib/Passes/IdenticalCodeFolding.cpp
@@ -413,7 +413,7 @@
   uint64_t NumFunctionsFolded = 0;
   std::atomic<uint64_t> NumJTFunctionsFolded{0};
   std::atomic<uint64_t> BytesSavedEstimate{0};
-  std::atomic<uint64_t> CallsSavedEstimate{0};
+  std::atomic<uint64_t> NumCalled{0};
   std::atomic<uint64_t> NumFoldedLastIteration{0};
   CongruentBucketsMap CongruentBuckets;
 
@@ -493,6 +493,8 @@
             });
 
         BinaryFunction *ParentBF = Twins[0];
+        if (!ParentBF->isMerged())
+          NumCalled += ParentBF->getKnownExecutionCount();
         for (unsigned I = 1; I < Twins.size(); ++I) {
           BinaryFunction *ChildBF = Twins[I];
           LLVM_DEBUG(dbgs() << "BOLT-DEBUG: folding " << *ChildBF << " into "
@@ -506,8 +508,8 @@
 
           // Fold the function and remove from the list of processed functions.
           BytesSavedEstimate += ChildBF->getSize();
-          CallsSavedEstimate += std::min(ChildBF->getKnownExecutionCount(),
-                                         ParentBF->getKnownExecutionCount());
+          if (!ChildBF->isMerged())
+            NumCalled += ChildBF->getKnownExecutionCount();
           BC.foldFunction(*ChildBF, *ParentBF);
 
           ++NumFoldedLastIteration;
@@ -580,7 +582,7 @@
            << "BOLT-INFO: Removing all identical functions will save "
            << format("%.2lf", (double)BytesSavedEstimate / 1024)
            << " KB of code space. Folded functions were called "
-           << CallsSavedEstimate << " times based on profile.\n";
+           << NumCalled << " times based on profile.\n";
 }
 
 } // namespace bolt
Index: bolt/lib/Core/BinaryContext.cpp
===================================================================
--- bolt/lib/Core/BinaryContext.cpp
+++ bolt/lib/Core/BinaryContext.cpp
@@ -1345,6 +1345,8 @@
 
     ChildBF.setFolded(&ParentBF);
   }
+
+  ParentBF.setMerged();
 }
 
 void BinaryContext::fixBinaryDataHoles() {
Index: bolt/include/bolt/Core/BinaryFunction.h
===================================================================
--- bolt/include/bolt/Core/BinaryFunction.h
+++ bolt/include/bolt/Core/BinaryFunction.h
@@ -349,6 +349,9 @@
   /// This attribute is only valid when hasCFG() == true.
   bool HasCanonicalCFG{true};
 
+  /// True if another function body was merged into this one.
+  bool IsMerged{false};
+
   /// Name for the section this function code should reside in.
   std::string CodeSectionName;
 
@@ -1407,6 +1410,9 @@
   /// Return true if the body of the function was merged into another function.
   bool isFolded() const { return FoldedIntoFunction != nullptr; }
 
+  /// Return true if other functions were merged into this one.
+  bool isMerged() const { return IsMerged; }
+
   /// If this function was folded, return the function it was folded into.
   BinaryFunction *getFoldedIntoFunction() const { return FoldedIntoFunction; }
 
@@ -1770,6 +1776,9 @@
 
   void setFolded(BinaryFunction *BF) { FoldedIntoFunction = BF; }
 
+  /// Indicate that another function body was merged with this function.
+  void setMerged() { IsMerged = true; }
+
   BinaryFunction &setPersonalityFunction(uint64_t Addr) {
     assert(!PersonalityFunction && "can't set personality function twice");
     PersonalityFunction = BC.getOrCreateGlobalSymbol(Addr, "FUNCat");


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D144807.500496.patch
Type: text/x-patch
Size: 3460 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230226/2b10bafc/attachment.bin>