[BOLT] Remove mutable from BB::LayoutIndex

[shawbyoung]

Removed mutability from BB::LayoutIndex, subsequently removed const from BB::SetLayout, and changed BF::dfs to track visited blocks with a set as opposed to tracking and altering LayoutIndexes for more consistent code.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: shaw young (shawbyoung)

Changes

Removed mutability from BB::LayoutIndex, subsequently removed const from BB::SetLayout, and changed BF::dfs to track visited blocks with a set as opposed to tracking and altering LayoutIndexes for more consistent code.

---

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

2 Files Affected:

• (modified) bolt/include/bolt/Core/BinaryBasicBlock.h (+2-2)
• (modified) bolt/lib/Core/BinaryFunction.cpp (+3-7)

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()) {

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[aaupov]

Please format the commit message to 72 characters per line.
Minor nit: I'd also replace "BB:LayoutIndex" with "BB::LayoutIndex" as more idiomatic version.

Otherwise looks good to me, thank you for working on it! Let's wait for internal testing to finish before landing.

[aaupov]

Test using -icf-dfs fails NFC checks. Need investigation.

[aaupov]

Thank you for working on this. The diff is moving in the right direction, but as evidenced by the amount of code to avoid setting BB->LayoutIndex and associated overheads, it's not a good idea and we should still set it.

Please define a helper that given a block order sets BB->LayoutIndex, as part of FunctionLayout class.