[BOLT] Remove old layout from function layout

To track whether a function's new layout is different from its old
layout when updating it, the old layout would be kept around in memory
indefinitely (if the new layout is different). This was used only for
debugging/logging purposes. This patch forces the caller of function
layout's update method to copy the old layout into a temporary if they
need it by removing the old layout fields.

Reviewed By: rafauler

Differential Revision: https://reviews.llvm.org/D131413

bolt/include/bolt/Core/FunctionLayout.h

@@ -145,8 +145,6 @@ class FunctionLayout {
   /// `Fragments.size()` equals `this->size() + 1`. Always contains at least one
   /// fragment.
   FragmentListType Fragments = {0, 0};
-  BasicBlockListType PreviousBlocks;
-  FragmentListType PreviousFragments;
 
 public:
   /// Add an empty fragment.
@@ -174,12 +172,9 @@ class FunctionLayout {
 
   /// Replace the current layout with NewLayout. Uses the block's
   /// self-identifying fragment number to assign blocks to infer function
-  /// fragments.
-  void update(const ArrayRef<BinaryBasicBlock *> NewLayout);
-
-  /// Return true if the layout has been changed by basic block reordering,
-  /// false otherwise.
-  bool hasLayoutChanged() const { return !PreviousBlocks.empty(); }
+  /// fragments. Returns `true` if the new layout is different from the current
+  /// layout.
+  bool update(ArrayRef<BinaryBasicBlock *> NewLayout);
 
   /// Clear layout releasing memory.
   void clear();
@@ -196,7 +191,8 @@ class FunctionLayout {
 
   /// Get the edit distance of the new layout with respect to the previous
   /// layout after basic block reordering.
-  uint64_t getEditDistance() const;
+  uint64_t
+  getEditDistance(ArrayRef<const BinaryBasicBlock *> OldBlockOrder) const;
 
   /// True if the function is split into at most 2 fragments. Mostly used for
   /// checking whether a function can be processed in places that do not support

bolt/include/bolt/Passes/BinaryPasses.h

@@ -152,7 +152,9 @@ class ReorderBasicBlocks : public BinaryFunctionPass {
   };
 
 private:
-  void modifyFunctionLayout(BinaryFunction &Function, LayoutType Type,
+  /// Run the specified layout algorithm on the given function. Returns `true`
+  /// if the order of blocks was changed.
+  bool modifyFunctionLayout(BinaryFunction &Function, LayoutType Type,
                             bool MinBranchClusters) const;
 
 public:

bolt/lib/Core/FunctionLayout.cpp

@@ -76,18 +76,22 @@ void FunctionLayout::updateLayoutIndices() const {
   }
 }
 
-void FunctionLayout::update(const ArrayRef<BinaryBasicBlock *> NewLayout) {
-  PreviousBlocks = std::move(Blocks);
-  PreviousFragments = std::move(Fragments);
+bool FunctionLayout::update(const ArrayRef<BinaryBasicBlock *> NewLayout) {
+  const bool EqualBlockOrder = llvm::equal(Blocks, NewLayout);
+  if (EqualBlockOrder) {
+    const bool EqualPartitioning =
+        llvm::all_of(fragments(), [](const FunctionFragment FF) {
+          return llvm::all_of(FF, [&](const BinaryBasicBlock *const BB) {
+            return FF.Num == BB->getFragmentNum();
+          });
+        });
+    if (EqualPartitioning)
+      return false;
+  }
 
-  Blocks.clear();
+  Blocks = BasicBlockListType(NewLayout.begin(), NewLayout.end());
   Fragments = {0, 0};
 
-  if (NewLayout.empty())
-    return;
-
-  copy(NewLayout, std::back_inserter(Blocks));
-
   // Generate fragments
   for (const auto &BB : enumerate(Blocks)) {
     unsigned FragmentNum = BB.value()->getFragmentNum().get();
@@ -106,19 +110,12 @@ void FunctionLayout::update(const ArrayRef<BinaryBasicBlock *> NewLayout) {
     Fragments[FragmentNum + 1] = BB.index() + 1;
   }
 
-  if (PreviousBlocks == Blocks && PreviousFragments == Fragments) {
-    // If new layout is the same as previous layout, clear previous layout so
-    // hasLayoutChanged() returns false.
-    PreviousBlocks = {};
-    PreviousFragments = {};
-  }
+  return true;
 }
 
 void FunctionLayout::clear() {
   Blocks = {};
   Fragments = {0, 0};
-  PreviousBlocks = {};
-  PreviousFragments = {0, 0};
 }
 
 BinaryBasicBlock *FunctionLayout::getBasicBlockAfter(const BinaryBasicBlock *BB,
@@ -144,8 +141,9 @@ bool FunctionLayout::isSplit() const {
   return NonEmptyFragCount >= 2;
 }
 
-uint64_t FunctionLayout::getEditDistance() const {
-  return ComputeEditDistance<BinaryBasicBlock *>(PreviousBlocks, Blocks);
+uint64_t FunctionLayout::getEditDistance(
+    const ArrayRef<const BinaryBasicBlock *> OldBlockOrder) const {
+  return ComputeEditDistance<const BinaryBasicBlock *>(OldBlockOrder, Blocks);
 }
 
 FunctionLayout::block_const_iterator

bolt/lib/Passes/BinaryPasses.cpp

@@ -11,11 +11,13 @@
 //===----------------------------------------------------------------------===//
 
 #include "bolt/Passes/BinaryPasses.h"
+#include "bolt/Core/FunctionLayout.h"
 #include "bolt/Core/ParallelUtilities.h"
 #include "bolt/Passes/ReorderAlgorithm.h"
 #include "bolt/Passes/ReorderFunctions.h"
 #include "llvm/Support/CommandLine.h"
-
+#include <atomic>
+#include <mutex>
 #include <numeric>
 #include <vector>
 
@@ -388,12 +390,25 @@ void ReorderBasicBlocks::runOnFunctions(BinaryContext &BC) {
   if (opts::ReorderBlocks == ReorderBasicBlocks::LT_NONE)
     return;
 
-  std::atomic<uint64_t> ModifiedFuncCount{0};
+  std::atomic_uint64_t ModifiedFuncCount(0);
+  std::mutex FunctionEditDistanceMutex;
+  DenseMap<const BinaryFunction *, uint64_t> FunctionEditDistance;
 
   ParallelUtilities::WorkFuncTy WorkFun = [&](BinaryFunction &BF) {
-    modifyFunctionLayout(BF, opts::ReorderBlocks, opts::MinBranchClusters);
-    if (BF.getLayout().hasLayoutChanged())
-      ++ModifiedFuncCount;
+    SmallVector<const BinaryBasicBlock *, 0> OldBlockOrder;
+    if (opts::PrintFuncStat > 0)
+      llvm::copy(BF.getLayout().blocks(), std::back_inserter(OldBlockOrder));
+
+    const bool LayoutChanged =
+        modifyFunctionLayout(BF, opts::ReorderBlocks, opts::MinBranchClusters);
+    if (LayoutChanged) {
+      ModifiedFuncCount.fetch_add(1, std::memory_order_relaxed);
+      if (opts::PrintFuncStat > 0) {
+        const uint64_t Distance = BF.getLayout().getEditDistance(OldBlockOrder);
+        std::lock_guard<std::mutex> Lock(FunctionEditDistanceMutex);
+        FunctionEditDistance[&BF] = Distance;
+      }
+    }
   };
 
   ParallelUtilities::PredicateTy SkipFunc = [&](const BinaryFunction &BF) {
@@ -405,8 +420,9 @@ void ReorderBasicBlocks::runOnFunctions(BinaryContext &BC) {
       "ReorderBasicBlocks");
 
   outs() << "BOLT-INFO: basic block reordering modified layout of "
-         << format("%zu (%.2lf%%) functions\n", ModifiedFuncCount.load(),
-                   100.0 * ModifiedFuncCount.load() /
+         << format("%zu (%.2lf%%) functions\n",
+                   ModifiedFuncCount.load(std::memory_order_relaxed),
+                   100.0 * ModifiedFuncCount.load(std::memory_order_relaxed) /
                        BC.getBinaryFunctions().size());
 
   if (opts::PrintFuncStat > 0) {
@@ -421,7 +437,7 @@ void ReorderBasicBlocks::runOnFunctions(BinaryContext &BC) {
     OS << "\nBOLT-INFO: Printing Function Statistics:\n\n";
     OS << "           There are " << BFs.size() << " functions in total. \n";
     OS << "           Number of functions being modified: "
-       << ModifiedFuncCount.load() << "\n";
+       << ModifiedFuncCount.load(std::memory_order_relaxed) << "\n";
     OS << "           User asks for detailed information on top "
        << opts::PrintFuncStat << " functions. (Ranked by function score)"
        << "\n\n";
@@ -439,23 +455,23 @@ void ReorderBasicBlocks::runOnFunctions(BinaryContext &BC) {
       OS << "             There are " << Function.getInstructionCount()
          << " number of instructions in this function.\n";
       OS << "             The edit distance for this function is: "
-         << Function.getLayout().getEditDistance() << "\n\n";
+         << FunctionEditDistance.lookup(&Function) << "\n\n";
     }
   }
 }
 
-void ReorderBasicBlocks::modifyFunctionLayout(BinaryFunction &BF,
+bool ReorderBasicBlocks::modifyFunctionLayout(BinaryFunction &BF,
                                               LayoutType Type,
                                               bool MinBranchClusters) const {
   if (BF.size() == 0 || Type == LT_NONE)
-    return;
+    return false;
 
   BinaryFunction::BasicBlockOrderType NewLayout;
   std::unique_ptr<ReorderAlgorithm> Algo;
 
   // Cannot do optimal layout without profile.
   if (Type != LT_REVERSE && !BF.hasValidProfile())
-    return;
+    return false;
 
   if (Type == LT_REVERSE) {
     Algo.reset(new ReverseReorderAlgorithm());
@@ -500,7 +516,7 @@ void ReorderBasicBlocks::modifyFunctionLayout(BinaryFunction &BF,
 
   Algo->reorderBasicBlocks(BF, NewLayout);
 
-  BF.getLayout().update(NewLayout);
+  return BF.getLayout().update(NewLayout);
 }
 
 void FixupBranches::runOnFunctions(BinaryContext &BC) {