Skip to content

Commit 38c8a03

Browse files
mingmingl-llvmmingmingl-llvm
committed
resolve comments
1 parent 97103c6 commit 38c8a03

File tree

4 files changed

+105
-61
lines changed

4 files changed

+105
-61
lines changed

‎llvm/lib/Analysis/StaticDataProfileInfo.cpp‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,12 +34,19 @@ StringRef StaticDataProfileInfo::getConstantSectionPrefix(
3434
auto Count = getConstantProfileCount(C);
3535
if (!Count)
3636
return "";
37+
// The accummulated counter shows the constant is hot. Return 'hot' whether
38+
// this variable is seen by unprofiled functions or not.
3739
if (PSI->isHotCount(*Count))
3840
return "hot";
41+
// The constant is not hot, and seen by unprofiled functions. We don't want to
42+
// assign it to unlikely sections, even if the counter says 'cold'. So return
43+
// an empty prefix before checking whether the counter is cold.
3944
if (ConstantWithoutCounts.count(C))
4045
return "";
46+
// The accummulated counter shows the constant is cold. Return 'unlikely'.
4147
if (PSI->isColdCount(*Count))
4248
return "unlikely";
49+
// The counter says lukewarm. Return an empty prefix.
4350
return "";
4451
}
4552

‎llvm/lib/CodeGen/StaticDataAnnotator.cpp‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@
1111
// StaticDataSplitter pass.
1212
//
1313
// The StaticDataSplitter pass is a machine function pass. It analyzes data
14-
// hotness based on code and adds counters in the StaticDataProfileInfo.
14+
// hotness based on code and adds counters in StaticDataProfileInfo via its
15+
// wrapper pass StaticDataProfileInfoWrapper.
16+
// The StaticDataProfileInfoWrapper sits in the middle between the
17+
// StaticDataSplitter and StaticDataAnnotator passes.
1518
// The StaticDataAnnotator pass is a module pass. It iterates global variables
1619
// in the module, looks up counters from StaticDataProfileInfo and sets the
1720
// section prefix based on profiles.
@@ -38,6 +41,8 @@
3841

3942
using namespace llvm;
4043

44+
/// A module pass which iterates global variables in the module and annotates
45+
/// their section prefixes based on profile-driven analysis.
4146
class StaticDataAnnotator : public ModulePass {
4247
public:
4348
static char ID;
@@ -61,14 +66,6 @@ class StaticDataAnnotator : public ModulePass {
6166
bool runOnModule(Module &M) override;
6267
};
6368

64-
// Returns true if the global variable already has a section prefix that is the
65-
// same as `Prefix`.
66-
static bool alreadyHasSectionPrefix(const GlobalVariable &GV,
67-
StringRef Prefix) {
68-
std::optional<StringRef> SectionPrefix = GV.getSectionPrefix();
69-
return SectionPrefix && (*SectionPrefix == Prefix);
70-
}
71-
7269
bool StaticDataAnnotator::runOnModule(Module &M) {
7370
SDPI = &getAnalysis<StaticDataProfileInfoWrapperPass>()
7471
.getStaticDataProfileInfo();
@@ -82,8 +79,17 @@ bool StaticDataAnnotator::runOnModule(Module &M) {
8279
if (GV.isDeclarationForLinker())
8380
continue;
8481

82+
// The implementation below assumes prior passes don't set section prefixes,
83+
// and specifically do 'assign' rather than 'update'. So report error if a
84+
// section prefix is already set.
85+
if (auto maybeSectionPrefix = GV.getSectionPrefix();
86+
maybeSectionPrefix && !maybeSectionPrefix->empty())
87+
llvm::report_fatal_error("Global variable " + GV.getName() +
88+
" already has a section prefix " +
89+
*maybeSectionPrefix);
90+
8591
StringRef SectionPrefix = SDPI->getConstantSectionPrefix(&GV, PSI);
86-
if (SectionPrefix.empty() || alreadyHasSectionPrefix(GV, SectionPrefix))
92+
if (SectionPrefix.empty())
8793
continue;
8894

8995
GV.setSectionPrefix(SectionPrefix);

‎llvm/lib/CodeGen/StaticDataSplitter.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ class StaticDataSplitter : public MachineFunctionPass {
5656

5757
// Returns true if the global variable is in one of {.rodata, .bss, .data,
5858
// .data.rel.ro} sections.
59-
bool inStaticDataSection(const GlobalVariable *GV, const TargetMachine &TM);
59+
bool inStaticDataSection(const GlobalVariable &GV, const TargetMachine &TM);
6060

6161
// Returns the constant if the operand refers to a global variable or constant
6262
// that gets lowered to static data sections. Otherwise, return nullptr.
@@ -128,7 +128,8 @@ const Constant *StaticDataSplitter::getConstant(const MachineOperand &Op,
128128
const GlobalVariable *GV = getLocalLinkageGlobalVariable(Op.getGlobal());
129129
// Skip 'llvm.'-prefixed global variables conservatively because they are
130130
// often handled specially, and skip those not in static data sections.
131-
if (!GV || GV->getName().starts_with("llvm.") || !inStaticDataSection(GV, TM))
131+
if (!GV || GV->getName().starts_with("llvm.") ||
132+
!inStaticDataSection(*GV, TM))
132133
return nullptr;
133134
return GV;
134135
}
@@ -186,11 +187,10 @@ StaticDataSplitter::getLocalLinkageGlobalVariable(const GlobalValue *GV) {
186187
return (GV && GV->hasLocalLinkage()) ? dyn_cast<GlobalVariable>(GV) : nullptr;
187188
}
188189

189-
bool StaticDataSplitter::inStaticDataSection(const GlobalVariable *GV,
190+
bool StaticDataSplitter::inStaticDataSection(const GlobalVariable &GV,
190191
const TargetMachine &TM) {
191-
assert(GV && "Caller guaranteed");
192192

193-
SectionKind Kind = TargetLoweringObjectFile::getKindForGlobal(GV, TM);
193+
SectionKind Kind = TargetLoweringObjectFile::getKindForGlobal(&GV, TM);
194194
return Kind.isData() || Kind.isReadOnly() || Kind.isReadOnlyWithRel() ||
195195
Kind.isBSS();
196196
}

‎llvm/test/CodeGen/X86/global-variable-partition.ll‎

Lines changed: 77 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -14,89 +14,114 @@ target triple = "x86_64-unknown-linux-gnu"
1414
; RUN: llc -mtriple=x86_64-unknown-linux-gnu -enable-split-machine-functions \
1515
; RUN: -partition-static-data-sections=true -data-sections=true \
1616
; RUN: -unique-section-names=true -relocation-model=pic \
17-
; RUN: %s -o - 2>&1 | FileCheck %s --check-prefixes=SYM,DATA
17+
; RUN: %s -o - 2>&1 | FileCheck %s --check-prefixes=SYM,COMMON --dump-input=always
1818

1919
; This RUN command sets `-data-sections=true -unique-section-names=false` so
2020
; data sections are uniqufied by variable names.
2121
; RUN: llc -mtriple=x86_64-unknown-linux-gnu -enable-split-machine-functions \
2222
; RUN: -partition-static-data-sections=true -data-sections=true \
2323
; RUN: -unique-section-names=false -relocation-model=pic \
24-
; RUN: %s -o - 2>&1 | FileCheck %s --check-prefixes=UNIQ,DATA
24+
; RUN: %s -o - 2>&1 | FileCheck %s --check-prefixes=UNIQ,COMMON --dump-input=always
2525

2626
; This RUN command sets `-data-sections=false -unique-section-names=false`.
2727
; RUN: llc -mtriple=x86_64-unknown-linux-gnu -enable-split-machine-functions \
2828
; RUN: -partition-static-data-sections=true -data-sections=false \
2929
; RUN: -unique-section-names=false -relocation-model=pic \
30-
; RUN: %s -o - 2>&1 | FileCheck %s --check-prefixes=AGG,DATA
30+
; RUN: %s -o - 2>&1 | FileCheck %s --check-prefixes=AGG,COMMON --dump-input=always
3131

3232
; For @.str and @.str.1
33-
; SYM: .section .rodata.str1.1.hot.
34-
; UNIQ: .section .rodata.str1.1.hot.,"aMS",@progbits,1
35-
; AGG: .section .rodata.str1.1.hot
36-
; DATA: .L.str
37-
; DATA: "hot\t"
38-
; DATA: .L.str.1
39-
; DATA: "%d\t%d\t%d\n"
33+
; COMMON: .type .L.str,@object
34+
; SYM-NEXT: .section .rodata.str1.1.hot.
35+
; UNIQ-NEXT: .section .rodata.str1.1.hot.,"aMS",@progbits,1
36+
; AGG-NEXT: .section .rodata.str1.1.hot
37+
; COMMON-NEXT: .L.str:
38+
; COMMON-NEXT: "hot\t"
39+
; COMMON: .L.str.1:
40+
; COMMON-NEXT: "%d\t%d\t%d\n"
4041

4142
; For @hot_relro_array
42-
; SYM: .section .data.rel.ro.hot.hot_relro_array
43-
; UNIQ: .section .data.rel.ro.hot.,"aw",@progbits,unique,3
44-
; AGG: .section .data.rel.ro.hot.,"aw",@progbits
43+
; COMMON: .type hot_relro_array,@object
44+
; SYM-NEXT: .section .data.rel.ro.hot.hot_relro_array
45+
; UNIQ-NEXT: .section .data.rel.ro.hot.,"aw",@progbits,unique,3
46+
; AGG-NEXT: .section .data.rel.ro.hot.,"aw",@progbits
4547

4648
; For @hot_data, which is accessed by {cold_func, unprofiled_func, hot_func}.
47-
; SYM: .section .data.hot.hot_data,"aw",@progbits
48-
; UNIQ: .section .data.hot.,"aw",@progbits,unique,4
49-
; AGG: .section .data.hot.,"aw",@progbits
49+
; COMMON: .type hot_data,@object
50+
; SYM-NEXT: .section .data.hot.hot_data,"aw",@progbits
51+
; UNIQ-NEXT: .section .data.hot.,"aw",@progbits,unique,4
52+
; AGG-NEXT: .section .data.hot.,"aw",@progbits
5053

5154
; For @hot_bss, which is accessed by {unprofiled_func, hot_func}.
52-
; SYM: .section .bss.hot.hot_bss,"aw",@nobits
53-
; UNIQ: .section .bss.hot.,"aw",@nobits,unique,5
54-
; AGG: .section .bss.hot.,"aw",@nobits
55+
; COMMON: .type hot_bss,@object
56+
; SYM-NEXT: .section .bss.hot.hot_bss,"aw",@nobits
57+
; UNIQ-NEXT: .section .bss.hot.,"aw",@nobits,unique,5
58+
; AGG-NEXT: .section .bss.hot.,"aw",@nobits
5559

5660
; For @.str.2
57-
; SYM: .section .rodata.str1.1.unlikely.,"aMS",@progbits,1
58-
; UNIQ: section .rodata.str1.1.unlikely.,"aMS",@progbits,1
59-
; AGG: .section .rodata.str1.1.unlikely.,"aMS",@progbits,1
60-
; DATA: .L.str.2:
61-
; DATA: "cold%d\t%d\t%d\n"
61+
; COMMON: .type .L.str.2,@object
62+
; SYM-NEXT: .section .rodata.str1.1.unlikely.,"aMS",@progbits,1
63+
; UNIQ-NEXT: .section .rodata.str1.1.unlikely.,"aMS",@progbits,1
64+
; AGG-NEXT: .section .rodata.str1.1.unlikely.,"aMS",@progbits,1
65+
; COMMON-NEXT: .L.str.2:
66+
; COMMON-NEXT: "cold%d\t%d\t%d\n"
6267

6368
; For @cold_bss
64-
; SYM: .section .bss.unlikely.cold_bss,"aw",@nobits
65-
; UNIQ: .section .bss.unlikely.,"aw",@nobits,unique,6
66-
; AGG: .section .bss.unlikely.,"aw",@nobits
69+
; COMMON: .type cold_bss,@object
70+
; SYM-NEXT: .section .bss.unlikely.cold_bss,"aw",@nobits
71+
; UNIQ-NEXT: .section .bss.unlikely.,"aw",@nobits,unique,6
72+
; AGG-NEXT: .section .bss.unlikely.,"aw",@nobits
6773

6874
; For @cold_data
69-
; SYM: .section .data.unlikely.cold_data,"aw",@progbits
70-
; UNIQ: .section .data.unlikely.,"aw",@progbits,unique,7
71-
; AGG: .section .data.unlikely.,"aw",@progbits
75+
; COMMON: .type cold_data,@object
76+
; SYM-NEXT: .section .data.unlikely.cold_data,"aw",@progbits
77+
; UNIQ-NEXT: .section .data.unlikely.,"aw",@progbits,unique,7
78+
; AGG-NEXT: .section .data.unlikely.,"aw",@progbits
79+
80+
; For @cold_data_custom_foo_section
81+
; It has an explicit section 'foo' and shouldn't have hot or unlikely suffix.
82+
; COMMON: .type cold_data_custom_foo_section,@object
83+
; SYM-NEXT: .section foo,"aw",@progbits
84+
; UNIQ-NEXT: .section foo,"aw",@progbits
85+
; AGG-NEXT: .section foo,"aw",@progbits
7286

7387
; For @cold_relro_array
74-
; SYM: .section .data.rel.ro.unlikely.cold_relro_array,"aw",@progbits
75-
; UNIQ: .section .data.rel.ro.unlikely.,"aw",@progbits,unique,8
76-
; AGG: .section .data.rel.ro.unlikely.,"aw",@progbits
88+
; COMMON: .type cold_relro_array,@object
89+
; SYM-NEXT: .section .data.rel.ro.unlikely.cold_relro_array,"aw",@progbits
90+
; UNIQ-NEXT: .section .data.rel.ro.unlikely.,"aw",@progbits,unique,8
91+
; AGG-NEXT: .section .data.rel.ro.unlikely.,"aw",@progbits
7792

7893
; Currently static-data-splitter only analyzes access from code.
7994
; @bss2 and @data3 are indirectly accessed by code through @hot_relro_array
8095
; and @cold_relro_array. A follow-up item is to analyze indirect access via data
8196
; and prune the unlikely list.
8297
; For @bss2
83-
; SYM: .section .bss.unlikely.bss2,"aw",@nobits
84-
; UNIQ: .section .bss.unlikely.,"aw",@nobits,unique,9
85-
; AGG: .section .bss.unlikely.,"aw",@nobits
98+
; COMMON: .type bss2,@object
99+
; SYM-NEXT: .section .bss.unlikely.bss2,"aw",@nobits
100+
; UNIQ-NEXT: .section .bss.unlikely.,"aw",@nobits,unique,9
101+
; AGG-NEXT: .section .bss.unlikely.,"aw",@nobits
86102

87103
; For @data3
88-
; SYM: .section .data.unlikely.data3,"aw",@progbits
89-
; UNIQ: .section .data.unlikely.,"aw",@progbits,unique,10
90-
; AGG: .section .data.unlikely.,"aw",@progbits
104+
; COMMON: .type data3,@object
105+
; SYM-NEXT: .section .data.unlikely.data3,"aw",@progbits
106+
; UNIQ-NEXT: .section .data.unlikely.,"aw",@progbits,unique,10
107+
; AGG-NEXT: .section .data.unlikely.,"aw",@progbits
91108

92109
; For @data_with_unknown_hotness
93-
; SYM: .type .Ldata_with_unknown_hotness,@object # @data_with_unknown_hotness
94-
; SYM: .section .data..Ldata_with_unknown_hotness,"aw",@progbits
95-
; UNIQ: .section .data,"aw",@progbits,unique,11
110+
; SYM: .type .Ldata_with_unknown_hotness,@object # @data_with_unknown_hotness
111+
; SYM: .section .data..Ldata_with_unknown_hotness,"aw",@progbits
112+
; UNIQ: .section .data,"aw",@progbits,unique,11
96113
; The `.section` directive is omitted for .data with -unique-section-names=false.
97114
; See MCSectionELF::shouldOmitSectionDirective for the implementation details.
98-
; AGG: .data
99-
; DATA: .Ldata_with_unknown_hotness:
115+
; AGG: .data
116+
; COMMON: .Ldata_with_unknown_hotness:
117+
118+
; For @hot_data_custom_bar_section
119+
; It has an explicit section attribute 'var' and shouldn't have hot or unlikely suffix.
120+
; COMMON: .type hot_data_custom_bar_section,@object
121+
; SYM-NEXT: .section bar,"aw",@progbits
122+
; SYM: hot_data_custom_bar_section
123+
; UNIQ: .section bar,"aw",@progbits
124+
; AGG: .section bar,"aw",@progbits
100125

101126
@.str = private unnamed_addr constant [5 x i8] c"hot\09\00", align 1
102127
@.str.1 = private unnamed_addr constant [10 x i8] c"%d\09%d\09%d\0A\00", align 1
@@ -106,10 +131,12 @@ target triple = "x86_64-unknown-linux-gnu"
106131
@.str.2 = private unnamed_addr constant [14 x i8] c"cold%d\09%d\09%d\0A\00", align 1
107132
@cold_bss = internal global i32 0
108133
@cold_data = internal global i32 4
134+
@cold_data_custom_foo_section = internal global i32 100, section "foo"
109135
@cold_relro_array = internal constant [2 x ptr] [ptr @data3, ptr @bss2]
110136
@bss2 = internal global i32 0
111137
@data3 = internal global i32 3
112138
@data_with_unknown_hotness = private global i32 5
139+
@hot_data_custom_bar_section = internal global i32 101 #0
113140

114141
define void @cold_func(i32 %0) !prof !15 {
115142
%2 = load i32, ptr @cold_bss
@@ -121,7 +148,8 @@ define void @cold_func(i32 %0) !prof !15 {
121148
%8 = load i32, ptr %7
122149
%9 = load i32, ptr @data_with_unknown_hotness
123150
%11 = load i32, ptr @hot_data
124-
%12 = call i32 (...) @func_taking_arbitrary_param(ptr @.str.2, i32 %2, i32 %3, i32 %8, i32 %9, i32 %11)
151+
%12 = load i32, ptr @cold_data_custom_foo_section
152+
%13 = call i32 (...) @func_taking_arbitrary_param(ptr @.str.2, i32 %2, i32 %3, i32 %8, i32 %9, i32 %11, i32 %12)
125153
ret void
126154
}
127155

@@ -142,7 +170,8 @@ define void @hot_func(i32 %0) !prof !14 {
142170
%7 = load i32, ptr %6
143171
%8 = load i32, ptr @hot_data
144172
%9 = load i32, ptr @hot_bss
145-
%10 = call i32 (...) @func_taking_arbitrary_param(ptr @.str.1, i32 %7, i32 %8, i32 %9)
173+
%10 = load i32, ptr @hot_data_custom_bar_section
174+
%11 = call i32 (...) @func_taking_arbitrary_param(ptr @.str.1, i32 %7, i32 %8, i32 %9, i32 %10)
146175
ret void
147176
}
148177

@@ -178,6 +207,8 @@ define i32 @main(i32 %0, ptr %1) !prof !15 {
178207
declare i32 @rand()
179208
declare i32 @func_taking_arbitrary_param(...)
180209

210+
attributes #0 = {"data-section"="bar"}
211+
181212
!llvm.module.flags = !{!1}
182213

183214
!1 = !{i32 1, !"ProfileSummary", !2}

0 commit comments

Comments
 (0)