Skip to content

Conversation

@rlavaee
Copy link
Contributor

@rlavaee rlavaee commented Nov 10, 2025

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Nov 10, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-x86

Author: Rahman Lavaee (rlavaee)

Changes

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

5 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h (+6-10)
  • (modified) llvm/lib/CodeGen/BasicBlockSections.cpp (+10-13)
  • (modified) llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp (+9-7)
  • (modified) llvm/test/CodeGen/X86/basic-block-sections-list.ll (+8-54)
  • (modified) llvm/test/CodeGen/X86/basic-block-sections-source-drift.ll (+5-3)
diff --git a/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h b/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h
index 7b1a5f5019589..ee1f28377f7e4 100644
--- a/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h
+++ b/llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h
@@ -68,17 +68,13 @@ class BasicBlockSectionsProfileReader {
 
   BasicBlockSectionsProfileReader() = default;
 
-  // Returns true if basic block sections profile exist for function \p
-  // FuncName.
+  // Returns true if function \p FuncName is hot based on the basic block
+  // section profile.
   bool isFunctionHot(StringRef FuncName) const;
 
-  // Returns a pair with first element representing whether basic block sections
-  // profile exist for the function \p FuncName, and the second element
-  // representing the basic block sections profile (cluster info) for this
-  // function. If the first element is true and the second element is empty, it
-  // means unique basic block sections are desired for all basic blocks of the
-  // function.
-  std::pair<bool, SmallVector<BBClusterInfo>>
+  // Returns the cluster info for the function \p FuncName. Returns an empty
+  // vector if function has no cluster info.
+  SmallVector<BBClusterInfo>
   getClusterInfoForFunction(StringRef FuncName) const;
 
   // Returns the path clonings for the given function.
@@ -190,7 +186,7 @@ class BasicBlockSectionsProfileReaderWrapperPass : public ImmutablePass {
 
   bool isFunctionHot(StringRef FuncName) const;
 
-  std::pair<bool, SmallVector<BBClusterInfo>>
+  SmallVector<BBClusterInfo>
   getClusterInfoForFunction(StringRef FuncName) const;
 
   SmallVector<SmallVector<unsigned>>
diff --git a/llvm/lib/CodeGen/BasicBlockSections.cpp b/llvm/lib/CodeGen/BasicBlockSections.cpp
index e317e1c06741f..52e2909bec072 100644
--- a/llvm/lib/CodeGen/BasicBlockSections.cpp
+++ b/llvm/lib/CodeGen/BasicBlockSections.cpp
@@ -183,8 +183,7 @@ updateBranches(MachineFunction &MF,
 // clusters are ordered in increasing order of their IDs, with the "Exception"
 // and "Cold" succeeding all other clusters.
 // FuncClusterInfo represents the cluster information for basic blocks. It
-// maps from BBID of basic blocks to their cluster information. If this is
-// empty, it means unique sections for all basic blocks in the function.
+// maps from BBID of basic blocks to their cluster information.
 static void
 assignSections(MachineFunction &MF,
                const DenseMap<UniqueBBID, BBClusterInfo> &FuncClusterInfo) {
@@ -197,10 +196,8 @@ assignSections(MachineFunction &MF,
   for (auto &MBB : MF) {
     // With the 'all' option, every basic block is placed in a unique section.
     // With the 'list' option, every basic block is placed in a section
-    // associated with its cluster, unless we want individual unique sections
-    // for every basic block in this function (if FuncClusterInfo is empty).
-    if (MF.getTarget().getBBSectionsType() == llvm::BasicBlockSection::All ||
-        FuncClusterInfo.empty()) {
+    // associated with its cluster.
+    if (MF.getTarget().getBBSectionsType() == llvm::BasicBlockSection::All) {
       // If unique sections are desired for all basic blocks of the function, we
       // set every basic block's section ID equal to its original position in
       // the layout (which is equal to its number). This ensures that basic
@@ -308,22 +305,22 @@ bool BasicBlockSections::handleBBSections(MachineFunction &MF) {
   if (BBSectionsType == BasicBlockSection::List &&
       hasInstrProfHashMismatch(MF))
     return false;
-  // Renumber blocks before sorting them. This is useful for accessing the
-  // original layout positions and finding the original fallthroughs.
-  MF.RenumberBlocks();
 
   DenseMap<UniqueBBID, BBClusterInfo> FuncClusterInfo;
   if (BBSectionsType == BasicBlockSection::List) {
-    auto [HasProfile, ClusterInfo] =
-        getAnalysis<BasicBlockSectionsProfileReaderWrapperPass>()
-            .getClusterInfoForFunction(MF.getName());
-    if (!HasProfile)
+    auto ClusterInfo = getAnalysis<BasicBlockSectionsProfileReaderWrapperPass>()
+                           .getClusterInfoForFunction(MF.getName());
+    if (ClusterInfo.empty())
       return false;
     for (auto &BBClusterInfo : ClusterInfo) {
       FuncClusterInfo.try_emplace(BBClusterInfo.BBID, BBClusterInfo);
     }
   }
 
+  // Renumber blocks before sorting them. This is useful for accessing the
+  // original layout positions and finding the original fallthroughs.
+  MF.RenumberBlocks();
+
   MF.setBBSectionsType(BBSectionsType);
   assignSections(MF, FuncClusterInfo);
 
diff --git a/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp b/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
index 485b44ae4c4aa..c234c0f1b0b34 100644
--- a/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
+++ b/llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp
@@ -58,22 +58,24 @@ BasicBlockSectionsProfileReader::parseUniqueBBID(StringRef S) const {
 }
 
 bool BasicBlockSectionsProfileReader::isFunctionHot(StringRef FuncName) const {
-  return getClusterInfoForFunction(FuncName).first;
+  return !getClusterInfoForFunction(FuncName).empty();
 }
 
-std::pair<bool, SmallVector<BBClusterInfo>>
+SmallVector<BBClusterInfo>
 BasicBlockSectionsProfileReader::getClusterInfoForFunction(
     StringRef FuncName) const {
   auto R = ProgramPathAndClusterInfo.find(getAliasName(FuncName));
-  return R != ProgramPathAndClusterInfo.end()
-             ? std::pair(true, R->second.ClusterInfo)
-             : std::pair(false, SmallVector<BBClusterInfo>());
+  return R != ProgramPathAndClusterInfo.end() ? R->second.ClusterInfo
+                                              : SmallVector<BBClusterInfo>();
 }
 
 SmallVector<SmallVector<unsigned>>
 BasicBlockSectionsProfileReader::getClonePathsForFunction(
     StringRef FuncName) const {
-  return ProgramPathAndClusterInfo.lookup(getAliasName(FuncName)).ClonePaths;
+  auto R = ProgramPathAndClusterInfo.find(getAliasName(FuncName));
+  return R != ProgramPathAndClusterInfo.end()
+             ? R->second.ClonePaths
+             : SmallVector<SmallVector<unsigned>>();
 }
 
 uint64_t BasicBlockSectionsProfileReader::getEdgeCount(
@@ -494,7 +496,7 @@ bool BasicBlockSectionsProfileReaderWrapperPass::isFunctionHot(
   return BBSPR.isFunctionHot(FuncName);
 }
 
-std::pair<bool, SmallVector<BBClusterInfo>>
+SmallVector<BBClusterInfo>
 BasicBlockSectionsProfileReaderWrapperPass::getClusterInfoForFunction(
     StringRef FuncName) const {
   return BBSPR.getClusterInfoForFunction(FuncName);
diff --git a/llvm/test/CodeGen/X86/basic-block-sections-list.ll b/llvm/test/CodeGen/X86/basic-block-sections-list.ll
index 45ef452f4f5c1..d652a540f3e9c 100644
--- a/llvm/test/CodeGen/X86/basic-block-sections-list.ll
+++ b/llvm/test/CodeGen/X86/basic-block-sections-list.ll
@@ -1,17 +1,13 @@
-;; Check the basic block sections list option.
-;; version 0 profile:
-; RUN: echo '!_Z3foob' > %t1
+;; Check that specifying the function in the basic block sections profile
+;; without any other directives is a noop.
 ;;
-;; version 1 profile:
-; RUN: echo 'v1' > %t2
-; RUN: echo 'f _Z3foob' >> %t2
+;; Specify the bb sections profile:
+; RUN: echo 'v1' > %t
+; RUN: echo 'f _Z3foob' >> %t
 ;;
-; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=%t1 -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS --check-prefix=LINUX-SECTIONS-FUNCTION-SECTION
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t1 -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS --check-prefix=LINUX-SECTIONS-NO-FUNCTION-SECTION
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t1 -unique-basic-block-section-names --bbsections-guided-section-prefix=false | FileCheck %s -check-prefix=LINUX-SECTIONS-NO-GUIDED-PREFIX
-; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=%t2 -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS --check-prefix=LINUX-SECTIONS-FUNCTION-SECTION
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t2 -unique-basic-block-section-names | FileCheck %s -check-prefix=LINUX-SECTIONS --check-prefix=LINUX-SECTIONS-NO-FUNCTION-SECTION
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t2 -unique-basic-block-section-names --bbsections-guided-section-prefix=false | FileCheck %s -check-prefix=LINUX-SECTIONS-NO-GUIDED-PREFIX
+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=%t  > %bbsections
+; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections > %orig
+; RUN: diff -u %orig %bbsections
 
 define i32 @_Z3foob(i1 zeroext %0) nounwind {
   %2 = alloca i32, align 4
@@ -41,45 +37,3 @@ define i32 @_Z3foob(i1 zeroext %0) nounwind {
 
 declare i32 @_Z3barv() #1
 declare i32 @_Z3bazv() #1
-
-define i32 @_Z3zipb(i1 zeroext %0) nounwind {
-  %2 = alloca i32, align 4
-  %3 = alloca i8, align 1
-  %4 = zext i1 %0 to i8
-  store i8 %4, ptr %3, align 1
-  %5 = load i8, ptr %3, align 1
-  %6 = trunc i8 %5 to i1
-  %7 = zext i1 %6 to i32
-  %8 = icmp sgt i32 %7, 0
-  br i1 %8, label %9, label %11
-
-9:                                                ; preds = %1
-  %10 = call i32 @_Z3barv()
-  store i32 %10, ptr %2, align 4
-  br label %13
-
-11:                                               ; preds = %1
-  %12 = call i32 @_Z3bazv()
-  store i32 %12, ptr %2, align 4
-  br label %13
-
-13:                                               ; preds = %11, %9
-  %14 = load i32, ptr %2, align 4
-  ret i32 %14
-}
-
-; LINUX-SECTIONS-NO-GUIDED-PREFIX: .section        .text._Z3foob,"ax",@progbits
-; LINUX-SECTIONS: .section        .text.hot._Z3foob,"ax",@progbits
-; LINUX-SECTIONS: _Z3foob:
-; LINUX-SECTIONS: .section        .text.hot._Z3foob._Z3foob.__part.1,"ax",@progbits
-; LINUX-SECTIONS: _Z3foob.__part.1:
-; LINUX-SECTIONS: .section        .text.hot._Z3foob._Z3foob.__part.2,"ax",@progbits
-; LINUX-SECTIONS: _Z3foob.__part.2:
-; LINUX-SECTIONS: .section        .text.hot._Z3foob._Z3foob.__part.3,"ax",@progbits
-; LINUX-SECTIONS: _Z3foob.__part.3:
-
-; LINUX-SECTIONS-FUNCTION-SECTION: .section     .text._Z3zipb,"ax",@progbits
-; LINUX-SECTIONS-NO-FUNCTION-SECTION-NOT: .section     .text{{.*}}._Z3zipb,"ax",@progbits
-; LINUX-SECTIONS: _Z3zipb:
-; LINUX-SECTIONS-NOT: .section        .text{{.*}}._Z3zipb.__part.{{[0-9]+}},"ax",@progbits
-; LINUX-SECTIONS-NOT: _Z3zipb.__part.{{[0-9]+}}:
diff --git a/llvm/test/CodeGen/X86/basic-block-sections-source-drift.ll b/llvm/test/CodeGen/X86/basic-block-sections-source-drift.ll
index d481b147662dc..6e0db20ca0492 100644
--- a/llvm/test/CodeGen/X86/basic-block-sections-source-drift.ll
+++ b/llvm/test/CodeGen/X86/basic-block-sections-source-drift.ll
@@ -1,6 +1,8 @@
-; RUN: echo "!foo" > %t.order.txt
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t.order.txt  | FileCheck --check-prefix=SOURCE-DRIFT %s
-; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t.order.txt -bbsections-detect-source-drift=false | FileCheck --check-prefix=HASH-CHECK-DISABLED %s
+; RUN: echo "v1" > %t
+; RUN: echo "f foo" >> %t
+; RUN: echo "c 0" >> %t
+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t  | FileCheck --check-prefix=SOURCE-DRIFT %s
+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t -bbsections-detect-source-drift=false | FileCheck --check-prefix=HASH-CHECK-DISABLED %s
 
 define dso_local i32 @foo(i1 zeroext %0, i1 zeroext %1)  !annotation !1 {
   br i1 %0, label %5, label %3

@rlavaee rlavaee requested a review from tmsri November 10, 2025 18:12
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Nov 10, 2025
@rlavaee rlavaee merged commit 95db31e into llvm:main Nov 10, 2025
7 of 10 checks passed
Copy link
Member

@tmsri tmsri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


// Renumber blocks before sorting them. This is useful for accessing the
// original layout positions and finding the original fallthroughs.
MF.RenumberBlocks();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an unrelated change. Should we make this a separate one?

@slydiman
Copy link
Contributor

It seems LLDB buildbots lldb-remote-linux-ubuntu and lldb-remote-linux-win are broken after this patch.
Please take a look.

FAIL: lldb-api::TestStepUntilAPI.py

 File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/functionalities/thread/step_until/TestStepUntilAPI.py", line 115, in test_missing_discontinuous
    self._assertDiscontinuity()
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/functionalities/thread/step_until/TestStepUntilAPI.py", line 66, in _assertDiscontinuity
    self.assertTrue(
AssertionError: False is not true : 'foo' is not between 'call_me'     Module: file = "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/lldb-test-build.noindex/functionalities/thread/step_until/TestStepUntilAPI.test_missing_discontinuous/a.out", arch = "aarch64"
CompileUnit: id = {0x00000000}, file = "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/functionalities/thread/step_until/main.c", language = "c11"
   Function: id = {0x0000005f}, name = "foo", range = [0x00000000000107b8-0x00000000000107cc)
   FuncType: id = {0x0000005f}, byte-size = 0, decl = main.c:8, compiler_type = "int (int)"
     Symbol: id = {0x00000041}, range = [0x00000000000107b8-0x00000000000107cc), name="foo"     Module: file = "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/lldb-test-build.noindex/functionalities/thread/step_until/TestStepUntilAPI.test_missing_discontinuous/a.out", arch = "aarch64"
CompileUnit: id = {0x00000000}, file = "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/functionalities/thread/step_until/main.c", language = "c11"
   Function: id = {0x0000007a}, name = "call_me", range = [0x0000000000010760-0x00000000000107b8)
   FuncType: id = {0x0000007a}, byte-size = 0, decl = main.c:10, compiler_type = "int (int)"
     Symbol: id = {0x00000042}, range = [0x0000000000010760-0x00000000000107b8), name="call_me"

@rlavaee
Copy link
Contributor Author

rlavaee commented Nov 11, 2025

It seems LLDB buildbots lldb-remote-linux-ubuntu and lldb-remote-linux-win are broken after this patch. Please take a look.

FAIL: lldb-api::TestStepUntilAPI.py

 File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/functionalities/thread/step_until/TestStepUntilAPI.py", line 115, in test_missing_discontinuous
    self._assertDiscontinuity()
  File "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/functionalities/thread/step_until/TestStepUntilAPI.py", line 66, in _assertDiscontinuity
    self.assertTrue(
AssertionError: False is not true : 'foo' is not between 'call_me'     Module: file = "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/lldb-test-build.noindex/functionalities/thread/step_until/TestStepUntilAPI.test_missing_discontinuous/a.out", arch = "aarch64"
CompileUnit: id = {0x00000000}, file = "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/functionalities/thread/step_until/main.c", language = "c11"
   Function: id = {0x0000005f}, name = "foo", range = [0x00000000000107b8-0x00000000000107cc)
   FuncType: id = {0x0000005f}, byte-size = 0, decl = main.c:8, compiler_type = "int (int)"
     Symbol: id = {0x00000041}, range = [0x00000000000107b8-0x00000000000107cc), name="foo"     Module: file = "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/build/lldb-test-build.noindex/functionalities/thread/step_until/TestStepUntilAPI.test_missing_discontinuous/a.out", arch = "aarch64"
CompileUnit: id = {0x00000000}, file = "/home/buildbot/worker/as-builder-9/lldb-remote-linux-ubuntu/llvm-project/lldb/test/API/functionalities/thread/step_until/main.c", language = "c11"
   Function: id = {0x0000007a}, name = "call_me", range = [0x0000000000010760-0x00000000000107b8)
   FuncType: id = {0x0000007a}, byte-size = 0, decl = main.c:10, compiler_type = "int (int)"
     Symbol: id = {0x00000042}, range = [0x0000000000010760-0x00000000000107b8), name="call_me"

Thanks for reporting. The fix is here: #167423

@slydiman
Copy link
Contributor

slydiman commented Nov 11, 2025

Thanks for reporting. The fix is here: #167423

Nop.
https://lab.llvm.org/buildbot/#/builders/195/builds/17138 and https://lab.llvm.org/buildbot/#/builders/197/builds/10707 are still broken.

@rlavaee
Copy link
Contributor Author

rlavaee commented Nov 11, 2025

Thanks for reporting. The fix is here: #167423

Nop. https://lab.llvm.org/buildbot/#/builders/195/builds/17138 and https://lab.llvm.org/buildbot/#/builders/197/builds/10707 are still broken.

It looks like the test is asserting something more specific. How can I run this test? Running ninja check-lldb-api-functionalities-thread-step_until gives me

Total Discovered Tests: 2
  Unsupported: 2 (100.00%)

@DavidSpickett
Copy link
Collaborator

$ ./bin/lldb-dotest -p TestStepUntilAPI.py

And it'll tell you why they were unsupported.

I'm looking at the failure on AArch64 Linux right now.

@DavidSpickett
Copy link
Collaborator

Before:

>>> lldb.target.FindFunctions("foo")[0].function.GetRanges()
[[0xaaaaaaaa0684-0xaaaaaaaa0698)]
>>> lldb.target.FindFunctions("call_me")[0].function.GetRanges()
[[0xaaaaaaaa0650-0xaaaaaaaa0684), [0xaaaaaaaa0698-0xaaaaaaaa06c0)]

After:

>>> lldb.target.FindFunctions("foo")[0].function.GetRanges()
[[0xaaaaaaaa06a0-0xaaaaaaaa06b4)]
>>> lldb.target.FindFunctions("call_me")[0].function.GetRanges()
[[0xaaaaaaaa0650-0xaaaaaaaa06a0)]

The test is expecting to see call_me wrapped around foo.

DavidSpickett added a commit that referenced this pull request Nov 11, 2025
After #167359 / 95db31e.

A fix was attempted in #167423 but was not quite enough.

From what I could understand, in v1 format you have to specify
all the basic blocks. Where before !call_me implied they were all
cold (I think, very shaky understanding here).

For this test we want to see blocks like call_me/foo/call_me.
So adding a line for block 1 fixes the tests.

It could produce more blocks at some point but I think as long
as foo is within two of them, it'll be fine.
@DavidSpickett
Copy link
Collaborator

I've pushed a fix.

Given that there is no way to opt back into "v0" behaviour that I can find, and this changes behaviour, this should probably have a release note. Unless this option is too niche to have it, but please justify your choice to do so or not.

@rlavaee
Copy link
Contributor Author

rlavaee commented Nov 11, 2025

I've pushed a fix.

Given that there is no way to opt back into "v0" behaviour that I can find, and this changes behaviour, this should probably have a release note. Unless this option is too niche to have it, but please justify your choice to do so or not.

Thanks for fixing this. I did suspect that there would be test use cases for this option, given the ease of use (you don't need to specify each basic block). In this case, we can't use -fbasic-block-sections=all because it will create basic blocks for foo too. We don't think there are any operational use cases for this though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:X86 clang Clang issues not falling into any other category llvm:codegen

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants