[llvm] [BOLT] Remove mutable from BB:LayoutIndex (PR #93224)

shaw young via llvm-commits llvm-commits at lists.llvm.org
Tue May 28 09:36:22 PDT 2024


https://github.com/shawbyoung updated https://github.com/llvm/llvm-project/pull/93224

>From 4486b9c1a135fd1c758ac30379a133366fcd9917 Mon Sep 17 00:00:00 2001
From: shawbyoung <shawbyoung at gmail.com>
Date: Thu, 23 May 2024 10:58:35 -0700
Subject: [PATCH 1/4] [BOLT][NFC] Remove mutable from BB:LayoutIndex

---
 bolt/include/bolt/Core/BinaryBasicBlock.h |  4 ++--
 bolt/lib/Core/BinaryFunction.cpp          | 10 +++-------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/bolt/include/bolt/Core/BinaryBasicBlock.h b/bolt/include/bolt/Core/BinaryBasicBlock.h
index bc95e2c4de3a1..a57b70714fe38 100644
--- a/bolt/include/bolt/Core/BinaryBasicBlock.h
+++ b/bolt/include/bolt/Core/BinaryBasicBlock.h
@@ -115,7 +115,7 @@ class BinaryBasicBlock {
   unsigned Index{InvalidIndex};
 
   /// Index in the current layout.
-  mutable unsigned LayoutIndex{InvalidIndex};
+  unsigned LayoutIndex{InvalidIndex};
 
   /// Number of pseudo instructions in this block.
   uint32_t NumPseudos{0};
@@ -891,7 +891,7 @@ class BinaryBasicBlock {
   }
 
   /// Set layout index. To be used by BinaryFunction.
-  void setLayoutIndex(unsigned Index) const { LayoutIndex = Index; }
+  void setLayoutIndex(unsigned Index) { LayoutIndex = Index; }
 
   /// Needed by graph traits.
   BinaryFunction *getParent() const { return getFunction(); }
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 10b93e702984f..51dc5a2cbf0d6 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -3636,8 +3636,8 @@ bool BinaryFunction::forEachEntryPoint(EntryPointCallbackTy Callback) const {
 
 BinaryFunction::BasicBlockListType BinaryFunction::dfs() const {
   BasicBlockListType DFS;
-  unsigned Index = 0;
   std::stack<BinaryBasicBlock *> Stack;
+  std::set<BinaryBasicBlock *> Visited;
 
   // Push entry points to the stack in reverse order.
   //
@@ -3654,17 +3654,13 @@ BinaryFunction::BasicBlockListType BinaryFunction::dfs() const {
   for (BinaryBasicBlock *const BB : reverse(EntryPoints))
     Stack.push(BB);
 
-  for (BinaryBasicBlock &BB : blocks())
-    BB.setLayoutIndex(BinaryBasicBlock::InvalidIndex);
-
   while (!Stack.empty()) {
     BinaryBasicBlock *BB = Stack.top();
     Stack.pop();
 
-    if (BB->getLayoutIndex() != BinaryBasicBlock::InvalidIndex)
+    if (Visited.find(BB) != Visited.end())
       continue;
-
-    BB->setLayoutIndex(Index++);
+    Visited.insert(BB);
     DFS.push_back(BB);
 
     for (BinaryBasicBlock *SuccBB : BB->landing_pads()) {

>From c269abc0dc72bbe45b86a7ad79f8a68eb1ecef8d Mon Sep 17 00:00:00 2001
From: shawbyoung <shawbyoung at gmail.com>
Date: Fri, 24 May 2024 14:46:58 -0700
Subject: [PATCH 2/4] [BOLT][NFC] Fix invalid BB::getLayoutIndex calls

---
 bolt/lib/Passes/IdenticalCodeFolding.cpp | 28 ++++++++++++++++++++++--
 bolt/lib/Profile/YAMLProfileWriter.cpp   | 13 +++++++++--
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/bolt/lib/Passes/IdenticalCodeFolding.cpp b/bolt/lib/Passes/IdenticalCodeFolding.cpp
index 87eba10354a37..e43c7e1a32c20 100644
--- a/bolt/lib/Passes/IdenticalCodeFolding.cpp
+++ b/bolt/lib/Passes/IdenticalCodeFolding.cpp
@@ -180,11 +180,26 @@ static bool isIdenticalWith(const BinaryFunction &A, const BinaryFunction &B,
 
   const BinaryContext &BC = A.getBinaryContext();
 
+  std::unordered_map<const BinaryBasicBlock*, unsigned> LayoutIndiciesA;
+  for (unsigned I = 0; I < OrderA.size(); I++)
+    LayoutIndiciesA[OrderA[I]] = I;
+
+  std::unordered_map<const BinaryBasicBlock*, unsigned> LayoutIndiciesB;
+  for (unsigned I = 0; I < OrderB.size(); I++)
+    LayoutIndiciesB[OrderB[I]] = I;
+
   auto BBI = OrderB.begin();
   for (const BinaryBasicBlock *BB : OrderA) {
     const BinaryBasicBlock *OtherBB = *BBI;
 
-    if (BB->getLayoutIndex() != OtherBB->getLayoutIndex())
+    auto LayoutIndiciesAIt = LayoutIndiciesA.find(BB);
+    assert(LayoutIndiciesAIt != LayoutIndiciesA.end());
+    unsigned BBLayoutIndex = LayoutIndiciesAIt->second;
+
+    auto LayoutIndiciesBIt = LayoutIndiciesB.find(OtherBB);
+    assert(LayoutIndiciesBIt != LayoutIndiciesB.end());
+    unsigned OtherBBLayoutIndex = LayoutIndiciesBIt->second;
+    if (BBLayoutIndex != OtherBBLayoutIndex)
       return false;
 
     // Compare successor basic blocks.
@@ -195,7 +210,16 @@ static bool isIdenticalWith(const BinaryFunction &A, const BinaryFunction &B,
     auto SuccBBI = OtherBB->succ_begin();
     for (const BinaryBasicBlock *SuccBB : BB->successors()) {
       const BinaryBasicBlock *SuccOtherBB = *SuccBBI;
-      if (SuccBB->getLayoutIndex() != SuccOtherBB->getLayoutIndex())
+
+      LayoutIndiciesAIt = LayoutIndiciesA.find(SuccBB);
+      assert(LayoutIndiciesAIt != LayoutIndiciesA.end());
+      unsigned SuccBBLayoutIndex = LayoutIndiciesAIt->second;
+
+      LayoutIndiciesBIt = LayoutIndiciesB.find(SuccOtherBB);
+      assert(LayoutIndiciesBIt != LayoutIndiciesB.end());
+      unsigned SuccOtherBBLayoutIndex = LayoutIndiciesBIt->second;
+
+      if (SuccBBLayoutIndex != SuccOtherBBLayoutIndex)
         return false;
       ++SuccBBI;
     }
diff --git a/bolt/lib/Profile/YAMLProfileWriter.cpp b/bolt/lib/Profile/YAMLProfileWriter.cpp
index ef04ba0d21ad7..f0721a2a5d16b 100644
--- a/bolt/lib/Profile/YAMLProfileWriter.cpp
+++ b/bolt/lib/Profile/YAMLProfileWriter.cpp
@@ -69,9 +69,15 @@ YAMLProfileWriter::convert(const BinaryFunction &BF, bool UseDFS,
   llvm::copy(UseDFS ? BF.dfs() : BF.getLayout().blocks(),
              std::back_inserter(Order));
 
+  std::unordered_map<const BinaryBasicBlock*, unsigned> LayoutIndicies;
+  for (unsigned I = 0; I < Order.size(); I++)
+    LayoutIndicies[Order[I]] = I;
+
   for (const BinaryBasicBlock *BB : Order) {
     yaml::bolt::BinaryBasicBlockProfile YamlBB;
-    YamlBB.Index = BB->getLayoutIndex();
+    auto It = LayoutIndicies.find(BB);
+    assert(It != LayoutIndicies.end());
+    YamlBB.Index = It->second;
     YamlBB.NumInstructions = BB->getNumNonPseudos();
     YamlBB.Hash = BB->getHash();
 
@@ -160,7 +166,10 @@ YAMLProfileWriter::convert(const BinaryFunction &BF, bool UseDFS,
     auto BranchInfo = BB->branch_info_begin();
     for (const BinaryBasicBlock *Successor : BB->successors()) {
       yaml::bolt::SuccessorInfo YamlSI;
-      YamlSI.Index = Successor->getLayoutIndex();
+
+      auto It = LayoutIndicies.find(Successor);
+      assert(It != LayoutIndicies.end());
+      YamlSI.Index = It->second;
       YamlSI.Count = BranchInfo->Count;
       YamlSI.Mispreds = BranchInfo->MispredictedCount;
 

>From 44217bb26512fa6689e091185f76bb13eba9b154 Mon Sep 17 00:00:00 2001
From: shawbyoung <shawbyoung at gmail.com>
Date: Fri, 24 May 2024 14:53:12 -0700
Subject: [PATCH 3/4] Formatting

---
 bolt/lib/Passes/IdenticalCodeFolding.cpp | 4 ++--
 bolt/lib/Profile/YAMLProfileWriter.cpp   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/bolt/lib/Passes/IdenticalCodeFolding.cpp b/bolt/lib/Passes/IdenticalCodeFolding.cpp
index e43c7e1a32c20..1c2f2470824f5 100644
--- a/bolt/lib/Passes/IdenticalCodeFolding.cpp
+++ b/bolt/lib/Passes/IdenticalCodeFolding.cpp
@@ -180,11 +180,11 @@ static bool isIdenticalWith(const BinaryFunction &A, const BinaryFunction &B,
 
   const BinaryContext &BC = A.getBinaryContext();
 
-  std::unordered_map<const BinaryBasicBlock*, unsigned> LayoutIndiciesA;
+  std::unordered_map<const BinaryBasicBlock *, unsigned> LayoutIndiciesA;
   for (unsigned I = 0; I < OrderA.size(); I++)
     LayoutIndiciesA[OrderA[I]] = I;
 
-  std::unordered_map<const BinaryBasicBlock*, unsigned> LayoutIndiciesB;
+  std::unordered_map<const BinaryBasicBlock *, unsigned> LayoutIndiciesB;
   for (unsigned I = 0; I < OrderB.size(); I++)
     LayoutIndiciesB[OrderB[I]] = I;
 
diff --git a/bolt/lib/Profile/YAMLProfileWriter.cpp b/bolt/lib/Profile/YAMLProfileWriter.cpp
index f0721a2a5d16b..9aa2addec6c26 100644
--- a/bolt/lib/Profile/YAMLProfileWriter.cpp
+++ b/bolt/lib/Profile/YAMLProfileWriter.cpp
@@ -69,7 +69,7 @@ YAMLProfileWriter::convert(const BinaryFunction &BF, bool UseDFS,
   llvm::copy(UseDFS ? BF.dfs() : BF.getLayout().blocks(),
              std::back_inserter(Order));
 
-  std::unordered_map<const BinaryBasicBlock*, unsigned> LayoutIndicies;
+  std::unordered_map<const BinaryBasicBlock *, unsigned> LayoutIndicies;
   for (unsigned I = 0; I < Order.size(); I++)
     LayoutIndicies[Order[I]] = I;
 

>From 5fcb2604c6a3057b072397eb213ab52d0c5043e8 Mon Sep 17 00:00:00 2001
From: shawbyoung <shawbyoung at gmail.com>
Date: Tue, 28 May 2024 09:30:30 -0700
Subject: [PATCH 4/4] [BOLT] Using dfs to initialize buckets in ICF

Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:


Differential Revision: https://phabricator.intern.facebook.com/D57867071
---
 bolt/lib/Passes/IdenticalCodeFolding.cpp | 32 +++++-------------------
 bolt/lib/Profile/YAMLProfileWriter.cpp   | 12 ++-------
 2 files changed, 8 insertions(+), 36 deletions(-)

diff --git a/bolt/lib/Passes/IdenticalCodeFolding.cpp b/bolt/lib/Passes/IdenticalCodeFolding.cpp
index 1c2f2470824f5..d8b19bc7d3860 100644
--- a/bolt/lib/Passes/IdenticalCodeFolding.cpp
+++ b/bolt/lib/Passes/IdenticalCodeFolding.cpp
@@ -179,27 +179,11 @@ static bool isIdenticalWith(const BinaryFunction &A, const BinaryFunction &B,
   }
 
   const BinaryContext &BC = A.getBinaryContext();
-
-  std::unordered_map<const BinaryBasicBlock *, unsigned> LayoutIndiciesA;
-  for (unsigned I = 0; I < OrderA.size(); I++)
-    LayoutIndiciesA[OrderA[I]] = I;
-
-  std::unordered_map<const BinaryBasicBlock *, unsigned> LayoutIndiciesB;
-  for (unsigned I = 0; I < OrderB.size(); I++)
-    LayoutIndiciesB[OrderB[I]] = I;
-
   auto BBI = OrderB.begin();
   for (const BinaryBasicBlock *BB : OrderA) {
     const BinaryBasicBlock *OtherBB = *BBI;
 
-    auto LayoutIndiciesAIt = LayoutIndiciesA.find(BB);
-    assert(LayoutIndiciesAIt != LayoutIndiciesA.end());
-    unsigned BBLayoutIndex = LayoutIndiciesAIt->second;
-
-    auto LayoutIndiciesBIt = LayoutIndiciesB.find(OtherBB);
-    assert(LayoutIndiciesBIt != LayoutIndiciesB.end());
-    unsigned OtherBBLayoutIndex = LayoutIndiciesBIt->second;
-    if (BBLayoutIndex != OtherBBLayoutIndex)
+    if (BB->getLayoutIndex() != OtherBB->getLayoutIndex())
       return false;
 
     // Compare successor basic blocks.
@@ -211,15 +195,7 @@ static bool isIdenticalWith(const BinaryFunction &A, const BinaryFunction &B,
     for (const BinaryBasicBlock *SuccBB : BB->successors()) {
       const BinaryBasicBlock *SuccOtherBB = *SuccBBI;
 
-      LayoutIndiciesAIt = LayoutIndiciesA.find(SuccBB);
-      assert(LayoutIndiciesAIt != LayoutIndiciesA.end());
-      unsigned SuccBBLayoutIndex = LayoutIndiciesAIt->second;
-
-      LayoutIndiciesBIt = LayoutIndiciesB.find(SuccOtherBB);
-      assert(LayoutIndiciesBIt != LayoutIndiciesB.end());
-      unsigned SuccOtherBBLayoutIndex = LayoutIndiciesBIt->second;
-
-      if (SuccBBLayoutIndex != SuccOtherBBLayoutIndex)
+      if (SuccBB->getLayoutIndex() != SuccOtherBB->getLayoutIndex())
         return false;
       ++SuccBBI;
     }
@@ -408,6 +384,8 @@ Error IdenticalCodeFolding::runOnFunctions(BinaryContext &BC) {
       BinaryFunction &BF = BFI.second;
       if (!this->shouldOptimize(BF))
         continue;
+      if (opts::ICFUseDFS)
+        BF.getLayout().update(BF.dfs());
       CongruentBuckets[&BF].emplace(&BF);
     }
   };
@@ -433,6 +411,8 @@ Error IdenticalCodeFolding::runOnFunctions(BinaryContext &BC) {
       // Identical functions go into the same bucket.
       IdenticalBucketsMap IdenticalBuckets;
       for (BinaryFunction *BF : Candidates) {
+        if (opts::ICFUseDFS)
+          BF->getLayout().update(BF->dfs());
         IdenticalBuckets[BF].emplace_back(BF);
       }
 
diff --git a/bolt/lib/Profile/YAMLProfileWriter.cpp b/bolt/lib/Profile/YAMLProfileWriter.cpp
index 9aa2addec6c26..d27d6f7a8164c 100644
--- a/bolt/lib/Profile/YAMLProfileWriter.cpp
+++ b/bolt/lib/Profile/YAMLProfileWriter.cpp
@@ -69,15 +69,9 @@ YAMLProfileWriter::convert(const BinaryFunction &BF, bool UseDFS,
   llvm::copy(UseDFS ? BF.dfs() : BF.getLayout().blocks(),
              std::back_inserter(Order));
 
-  std::unordered_map<const BinaryBasicBlock *, unsigned> LayoutIndicies;
-  for (unsigned I = 0; I < Order.size(); I++)
-    LayoutIndicies[Order[I]] = I;
-
   for (const BinaryBasicBlock *BB : Order) {
     yaml::bolt::BinaryBasicBlockProfile YamlBB;
-    auto It = LayoutIndicies.find(BB);
-    assert(It != LayoutIndicies.end());
-    YamlBB.Index = It->second;
+    YamlBB.Index = BB->getLayoutIndex();
     YamlBB.NumInstructions = BB->getNumNonPseudos();
     YamlBB.Hash = BB->getHash();
 
@@ -167,9 +161,7 @@ YAMLProfileWriter::convert(const BinaryFunction &BF, bool UseDFS,
     for (const BinaryBasicBlock *Successor : BB->successors()) {
       yaml::bolt::SuccessorInfo YamlSI;
 
-      auto It = LayoutIndicies.find(Successor);
-      assert(It != LayoutIndicies.end());
-      YamlSI.Index = It->second;
+      YamlSI.Index = Successor->getLayoutIndex();
       YamlSI.Count = BranchInfo->Count;
       YamlSI.Mispreds = BranchInfo->MispredictedCount;
 



More information about the llvm-commits mailing list