Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BasicBlockSections] Allow mixing of -basic-block-sections with MFS. #117076

Merged
merged 1 commit into from
Nov 23, 2024
+87 −9
Merged

[BasicBlockSections] Allow mixing of -basic-block-sections with MFS. #117076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions llvm/lib/CodeGen/MachineFunctionSplitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "llvm/Analysis/EHUtils.h"
#include "llvm/Analysis/ProfileSummaryInfo.h"
#include "llvm/CodeGen/BasicBlockSectionUtils.h"
#include "llvm/CodeGen/BasicBlockSectionsProfileReader.h"
#include "llvm/CodeGen/MachineBasicBlock.h"
#include "llvm/CodeGen/MachineBlockFrequencyInfo.h"
#include "llvm/CodeGen/MachineFunction.h"
Expand Down Expand Up @@ -128,6 +129,9 @@ static bool isColdBlock(const MachineBasicBlock &MBB,
}

bool MachineFunctionSplitter::runOnMachineFunction(MachineFunction &MF) {
// Do not split functions when -basic-block-sections=all is specified.
if (MF.getTarget().getBBSectionsType() == llvm::BasicBlockSection::All)
return false;
Comment on lines +132 to +134
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you intend to support this combination in the future? I think basic block sections is useful for mapping in general beyond it's use in Propeller. If I recall correctly @boomanaiden154 mentioned this type of usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This applies for AutoFDO builds. The idea is to apply Propeller for functions whose BasicBlockAddressMap structure doesn't change since the profile was collected (even though those functions may have been optimized by autofdo).

Copy link
Contributor

Choose a reason for hiding this comment

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

For mapping, we just use the basic block address map rather than basic block sections as we only need to know where blocks are and their size rather than any ability to rearrange/GC them in the linker.

// We target functions with profile data. Static information in the form
// of exception handling code may be split to cold if user passes the
// mfs-split-ehcode flag.
Expand All @@ -139,6 +143,14 @@ bool MachineFunctionSplitter::runOnMachineFunction(MachineFunction &MF) {
if (!TII.isFunctionSafeToSplit(MF))
return false;

// Do not split functions with BasicBlockSections profiles as they will
// be split by the BasicBlockSections pass.
auto BBSectionsProfile =
getAnalysisIfAvailable<BasicBlockSectionsProfileReaderWrapperPass>();
if (BBSectionsProfile != nullptr &&
BBSectionsProfile->getBBSPR().isFunctionHot(MF.getName()))
return false;

// Renumbering blocks here preserves the order of the blocks as
// sortBasicBlocksAndUpdateBranches uses the numeric identifier to sort
// blocks. Preserving the order of blocks is essential to retaining decisions
Expand Down Expand Up @@ -201,6 +213,7 @@ void MachineFunctionSplitter::getAnalysisUsage(AnalysisUsage &AU) const {
AU.addRequired<MachineModuleInfoWrapperPass>();
AU.addRequired<MachineBlockFrequencyInfoWrapperPass>();
AU.addRequired<ProfileSummaryInfoWrapperPass>();
AU.addUsedIfAvailable<BasicBlockSectionsProfileReaderWrapperPass>();
}

char MachineFunctionSplitter::ID = 0;
Expand Down
17 changes: 9 additions & 8 deletions llvm/lib/CodeGen/TargetPassConfig.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1235,13 +1235,13 @@ void TargetPassConfig::addMachinePasses() {
addPass(createMIRAddFSDiscriminatorsPass(
sampleprof::FSDiscriminatorPass::PassLast));

bool NeedsBBSections =
TM->getBBSectionsType() != llvm::BasicBlockSection::None;
// Machine function splitter uses the basic block sections feature. Both
// cannot be enabled at the same time. We do not apply machine function
// splitter if -basic-block-sections is requested.
if (!NeedsBBSections && (TM->Options.EnableMachineFunctionSplitter ||
EnableMachineFunctionSplitter)) {
// Machine function splitter uses the basic block sections feature.
// When used along with `-basic-block-sections=`, the basic-block-sections
// feature takes precedence. This means functions eligible for
// basic-block-sections optimizations (`=all`, or `=list=` with function
// included in the list profile) will get that optimization instead.
if (TM->Options.EnableMachineFunctionSplitter ||
EnableMachineFunctionSplitter) {
const std::string ProfileFile = getFSProfileFile(TM);
if (!ProfileFile.empty()) {
if (EnableFSDiscriminator) {
Expand All @@ -1260,7 +1260,8 @@ void TargetPassConfig::addMachinePasses() {
}
// We run the BasicBlockSections pass if either we need BB sections or BB
// address map (or both).
if (NeedsBBSections || TM->Options.BBAddrMap) {
if (TM->getBBSectionsType() != llvm::BasicBlockSection::None ||
TM->Options.BBAddrMap) {
if (TM->getBBSectionsType() == llvm::BasicBlockSection::List) {
addPass(llvm::createBasicBlockSectionsProfileReaderWrapperPass(
TM->getBBSectionsFuncListBuf()));
Expand Down
66 changes: 65 additions & 1 deletion llvm/test/CodeGen/Generic/machine-function-splitter.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,21 @@
; REQUIRES: x86-registered-target

; COM: Machine function splitting with FDO profiles
; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -split-machine-functions | FileCheck %s -check-prefixes=MFS-DEFAULTS,MFS-DEFAULTS-X86
; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -split-machine-functions | FileCheck %s -check-prefixes=MFS-DEFAULTS,MFS-DEFAULTS-X86,MFS-NOBBSECTIONS
; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -split-machine-functions -mfs-psi-cutoff=0 -mfs-count-threshold=2000 | FileCheck %s --dump-input=always -check-prefixes=MFS-OPTS1,MFS-OPTS1-X86
; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -split-machine-functions -mfs-psi-cutoff=950000 | FileCheck %s -check-prefixes=MFS-OPTS2,MFS-OPTS2-X86
; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -split-machine-functions -mfs-split-ehcode | FileCheck %s -check-prefixes=MFS-EH-SPLIT,MFS-EH-SPLIT-X86
; RUN: llc < %s -mtriple=x86_64 -split-machine-functions -O0 -mfs-psi-cutoff=0 -mfs-count-threshold=10000 | FileCheck %s -check-prefixes=MFS-O0,MFS-O0-X86

; COM: Machine function splitting along with -basic-block-sections profile
; RUN: echo 'v1' > %t
; RUN: echo 'ffoo21' >> %t
; RUN: echo 'c0' >> %t
; RUN: echo 'ffoo22' >> %t
; RUN: echo 'c0 1' >> %t
; RUN: echo 'c2' >> %t
; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -basic-block-sections=%t -split-machine-functions | FileCheck %s --check-prefixes=MFS-BBSECTIONS

; RUN: llc < %s -mtriple=aarch64-unknown-linux-gnu -aarch64-min-jump-table-entries=4 -enable-split-machine-functions | FileCheck %s -check-prefixes=MFS-DEFAULTS,MFS-DEFAULTS-AARCH64
; RUN: llc < %s -mtriple=aarch64-unknown-linux-gnu -aarch64-min-jump-table-entries=4 -enable-split-machine-functions -mfs-psi-cutoff=0 -mfs-count-threshold=2000 | FileCheck %s --dump-input=always -check-prefixes=MFS-OPTS1,MFS-OPTS1-AARCH64
; RUN: llc < %s -mtriple=aarch64-unknown-linux-gnu -aarch64-min-jump-table-entries=4 -enable-split-machine-functions -mfs-psi-cutoff=950000 | FileCheck %s -check-prefixes=MFS-OPTS2,MFS-OPTS2-AARCH64
Expand Down Expand Up @@ -610,6 +619,61 @@ cold_asm_target:
ret void
}

define void @foo21(i1 zeroext %0) {
;; Check that a function with basic-block-sections profile (but no pgo profile)
;; is properly split when the profile is used along with mfs.
; MFS-BBSECTIONS: .section .text.hot.foo21
; MFS-NOBBSECTIONS-NOT: .section .text.hot.foo21
; MFS-BBSECTIONS-LABEL: foo21:
; MFS-NOBBSECTIONS-NOT: foo21.cold:
; MFS-BBSECTIONS: .section .text.split.foo21
; MFS-BBSECTIONS: foo21.cold
%2 = alloca i8, align 1
%3 = zext i1 %0 to i8
store i8 %3, ptr %2, align 1
%4 = load i8, ptr %2, align 1
%5 = trunc i8 %4 to i1
br i1 %5, label %6, label %8

6: ; preds = %1
%7 = call i32 @bar()
br label %10

8: ; preds = %1
%9 = call i32 @baz()
br label %10

10: ; preds = %8, %6
ret void
}

define void @foo22(i1 zeroext %0) nounwind !prof !14 !section_prefix !15 {
;; Check that when a function has both basic-block-section and pgo profiles
;; only the basic-block-section profile is used for splitting.

;; Check that we create two hot sections with -basic-block-sections.
; MFS-BBSECTIONS: .section .text.hot.foo22
; MFS-BBSECTIONS-LABEL: foo22:
; MFS-BBSECTIONS: callq bar
; MFS-BBSECTIONS: .section .text.hot.foo22
; MFS-BBSECTIONS-NEXT: foo22.__part.1:
; MFS-BBSECTIONS: callq baz
; MFS-BBSECTIONS-NOT: .section .text.split.foo22
br i1 %0, label %2, label %4, !prof !17

2: ; preds = %1
%3 = call i32 @bar()
br label %6

4: ; preds = %1
%5 = call i32 @baz()
br label %6

6: ; preds = %4, %2
%7 = tail call i32 @qux()
ret void
}

declare i32 @bar()
declare i32 @baz()
declare i32 @bam()
Expand Down
Loading