-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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.
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
[AsmPrinter][ELF] Support profile-guided section prefix for jump tables' (read-only) data sections #122215
[AsmPrinter][ELF] Support profile-guided section prefix for jump tables' (read-only) data sections #122215
Changes from 12 commits
5d207e9
34b6b9b
dd74827
8d3a985
1bacc51
8a85d1a
e54dacb
27ef86d
e816def
28defd8
7f3e473
1bef9b1
59a958a
2d06092
9134ffa
ea2d838
027ae56
fd0034a
a79a789
d569eff
be96277
b919de9
6bb4b32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2861,7 +2861,6 @@ void AsmPrinter::emitConstantPool() { | |
// Print assembly representations of the jump tables used by the current | ||
// function. | ||
void AsmPrinter::emitJumpTableInfo() { | ||
const DataLayout &DL = MF->getDataLayout(); | ||
const MachineJumpTableInfo *MJTI = MF->getJumpTableInfo(); | ||
if (!MJTI) return; | ||
if (MJTI->getEntryKind() == MachineJumpTableInfo::EK_Inline) return; | ||
|
@@ -2876,42 +2875,101 @@ void AsmPrinter::emitJumpTableInfo() { | |
MJTI->getEntryKind() == MachineJumpTableInfo::EK_LabelDifference32 || | ||
MJTI->getEntryKind() == MachineJumpTableInfo::EK_LabelDifference64, | ||
F); | ||
|
||
std::vector<unsigned> JumpTableIndices; | ||
if (!TM.Options.EnableStaticDataPartitioning) { | ||
for (unsigned JTI = 0, JTSize = JT.size(); JTI < JTSize; ++JTI) | ||
JumpTableIndices.push_back(JTI); | ||
emitJumpTables(JumpTableIndices, TLOF.getSectionForJumpTable(F, TM), | ||
JTInDiffSection, *MJTI); | ||
return; | ||
} | ||
|
||
// When static data partitioning is enabled, collect jump table entries that | ||
// go into the same section together to reduce the amount of section switch | ||
// statements. | ||
// | ||
// Iterate all jump tables, put hot jump table indices towards the beginning | ||
// of the vector, and cold jump table indices towards the end. Meanwhile | ||
// retain the relative orders of original jump tables within a hot or unlikely | ||
// section by reversing the cold jump table indices. | ||
int NextHotJumpTableIndex = 0, NextColdJumpTableIndex = JT.size() - 1; | ||
JumpTableIndices.resize(JT.size()); | ||
for (unsigned JTI = 0, JTSize = JT.size(); JTI < JTSize; ++JTI) { | ||
if (JT[JTI].Hotness == MachineFunctionDataHotness::Cold) | ||
JumpTableIndices[NextColdJumpTableIndex--] = JTI; | ||
else | ||
JumpTableIndices[NextHotJumpTableIndex++] = JTI; | ||
mingmingl-llvm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
if (NextHotJumpTableIndex != 0) { | ||
emitJumpTables( | ||
ArrayRef<unsigned>(JumpTableIndices).take_front(NextHotJumpTableIndex), | ||
TLOF.getSectionForJumpTable(F, TM, &JT[0]), JTInDiffSection, *MJTI); | ||
} | ||
ellishg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if (NextHotJumpTableIndex < (int)JT.size()) { | ||
ellishg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Reverse the order of cold jump tables indices. | ||
for (int L = NextHotJumpTableIndex, R = JT.size() - 1; L < R; ++L, --R) | ||
std::swap(JumpTableIndices[L], JumpTableIndices[R]); | ||
ellishg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
emitJumpTables( | ||
ArrayRef<unsigned>(JumpTableIndices) | ||
.take_back(JT.size() - NextHotJumpTableIndex), | ||
ellishg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
TLOF.getSectionForJumpTable( | ||
F, TM, &JT[JumpTableIndices[NextHotJumpTableIndex]]), | ||
JTInDiffSection, *MJTI); | ||
} | ||
|
||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't need this return? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed it. |
||
} | ||
|
||
void AsmPrinter::emitJumpTables(ArrayRef<unsigned> JumpTableIndices, | ||
MCSection *JumpTableSection, | ||
bool JTInDiffSection, | ||
const MachineJumpTableInfo &MJTI) { | ||
if (JumpTableIndices.empty()) | ||
return; | ||
|
||
const DataLayout &DL = MF->getDataLayout(); | ||
if (JTInDiffSection) { | ||
// Drop it in the readonly section. | ||
MCSection *ReadOnlySection = TLOF.getSectionForJumpTable(F, TM); | ||
OutStreamer->switchSection(ReadOnlySection); | ||
OutStreamer->switchSection(JumpTableSection); | ||
} | ||
|
||
emitAlignment(Align(MJTI->getEntryAlignment(DL))); | ||
emitAlignment(Align(MJTI.getEntryAlignment(MF->getDataLayout()))); | ||
|
||
// Jump tables in code sections are marked with a data_region directive | ||
// where that's supported. | ||
if (!JTInDiffSection) | ||
OutStreamer->emitDataRegion(MCDR_DataRegionJT32); | ||
|
||
for (unsigned JTI = 0, e = JT.size(); JTI != e; ++JTI) { | ||
const std::vector<MachineBasicBlock*> &JTBBs = JT[JTI].MBBs; | ||
const auto &JT = MJTI.getJumpTables(); | ||
for (unsigned Index = 0, e = JumpTableIndices.size(); Index != e; ++Index) { | ||
ellishg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const std::vector<MachineBasicBlock *> &JTBBs = | ||
mingmingl-llvm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
JT[JumpTableIndices[Index]].MBBs; | ||
|
||
// If this jump table was deleted, ignore it. | ||
if (JTBBs.empty()) continue; | ||
if (JTBBs.empty()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are some formatting changes here (and below) that make it hard to reason about what actually changed. Any way to separate them out into a separate PR easily? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. #124645 does the refactor. |
||
continue; | ||
|
||
// For the EK_LabelDifference32 entry, if using .set avoids a relocation, | ||
/// emit a .set directive for each unique entry. | ||
if (MJTI->getEntryKind() == MachineJumpTableInfo::EK_LabelDifference32 && | ||
if (MJTI.getEntryKind() == MachineJumpTableInfo::EK_LabelDifference32 && | ||
MAI->doesSetDirectiveSuppressReloc()) { | ||
SmallPtrSet<const MachineBasicBlock*, 16> EmittedSets; | ||
SmallPtrSet<const MachineBasicBlock *, 16> EmittedSets; | ||
const TargetLowering *TLI = MF->getSubtarget().getTargetLowering(); | ||
const MCExpr *Base = TLI->getPICJumpTableRelocBaseExpr(MF,JTI,OutContext); | ||
const MCExpr *Base = TLI->getPICJumpTableRelocBaseExpr( | ||
MF, JumpTableIndices[Index], OutContext); | ||
for (const MachineBasicBlock *MBB : JTBBs) { | ||
if (!EmittedSets.insert(MBB).second) | ||
continue; | ||
|
||
// .set LJTSet, LBB32-base | ||
const MCExpr *LHS = | ||
MCSymbolRefExpr::create(MBB->getSymbol(), OutContext); | ||
OutStreamer->emitAssignment(GetJTSetSymbol(JTI, MBB->getNumber()), | ||
MCBinaryExpr::createSub(LHS, Base, | ||
OutContext)); | ||
MCSymbolRefExpr::create(MBB->getSymbol(), OutContext); | ||
OutStreamer->emitAssignment( | ||
GetJTSetSymbol(JumpTableIndices[Index], MBB->getNumber()), | ||
MCBinaryExpr::createSub(LHS, Base, OutContext)); | ||
} | ||
} | ||
|
||
|
@@ -2923,27 +2981,27 @@ void AsmPrinter::emitJumpTableInfo() { | |
// FIXME: This doesn't have to have any specific name, just any randomly | ||
// named and numbered local label started with 'l' would work. Simplify | ||
// GetJTISymbol. | ||
OutStreamer->emitLabel(GetJTISymbol(JTI, true)); | ||
OutStreamer->emitLabel(GetJTISymbol(JumpTableIndices[Index], true)); | ||
|
||
MCSymbol* JTISymbol = GetJTISymbol(JTI); | ||
MCSymbol *JTISymbol = GetJTISymbol(JumpTableIndices[Index]); | ||
OutStreamer->emitLabel(JTISymbol); | ||
|
||
// Defer MCAssembler based constant folding due to a performance issue. The | ||
// label differences will be evaluated at write time. | ||
for (const MachineBasicBlock *MBB : JTBBs) | ||
emitJumpTableEntry(MJTI, MBB, JTI); | ||
emitJumpTableEntry(MJTI, MBB, JumpTableIndices[Index]); | ||
} | ||
|
||
if (EmitJumpTableSizesSection) | ||
emitJumpTableSizesSection(MJTI, F); | ||
emitJumpTableSizesSection(MJTI, MF->getFunction()); | ||
|
||
if (!JTInDiffSection) | ||
OutStreamer->emitDataRegion(MCDR_DataRegionEnd); | ||
} | ||
|
||
void AsmPrinter::emitJumpTableSizesSection(const MachineJumpTableInfo *MJTI, | ||
void AsmPrinter::emitJumpTableSizesSection(const MachineJumpTableInfo &MJTI, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to split out the pointer to ref changes as a separate PR and commit it as NFC. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a6aa936 should be the pointer-to-ref change. I rebased the PR as suggested. After the rebase this diff will not show up. |
||
const Function &F) const { | ||
const std::vector<MachineJumpTableEntry> &JT = MJTI->getJumpTables(); | ||
const std::vector<MachineJumpTableEntry> &JT = MJTI.getJumpTables(); | ||
|
||
if (JT.empty()) | ||
return; | ||
|
@@ -2991,17 +3049,17 @@ void AsmPrinter::emitJumpTableSizesSection(const MachineJumpTableInfo *MJTI, | |
|
||
/// EmitJumpTableEntry - Emit a jump table entry for the specified MBB to the | ||
/// current stream. | ||
void AsmPrinter::emitJumpTableEntry(const MachineJumpTableInfo *MJTI, | ||
void AsmPrinter::emitJumpTableEntry(const MachineJumpTableInfo &MJTI, | ||
const MachineBasicBlock *MBB, | ||
unsigned UID) const { | ||
assert(MBB && MBB->getNumber() >= 0 && "Invalid basic block"); | ||
const MCExpr *Value = nullptr; | ||
switch (MJTI->getEntryKind()) { | ||
switch (MJTI.getEntryKind()) { | ||
case MachineJumpTableInfo::EK_Inline: | ||
llvm_unreachable("Cannot emit EK_Inline jump table entry"); | ||
case MachineJumpTableInfo::EK_Custom32: | ||
Value = MF->getSubtarget().getTargetLowering()->LowerCustomJumpTableEntry( | ||
MJTI, MBB, UID, OutContext); | ||
&MJTI, MBB, UID, OutContext); | ||
break; | ||
case MachineJumpTableInfo::EK_BlockAddress: | ||
// EK_BlockAddress - Each entry is a plain address of block, e.g.: | ||
|
@@ -3035,7 +3093,7 @@ void AsmPrinter::emitJumpTableEntry(const MachineJumpTableInfo *MJTI, | |
// If the .set directive avoids relocations, this is emitted as: | ||
// .set L4_5_set_123, LBB123 - LJTI1_2 | ||
// .word L4_5_set_123 | ||
if (MJTI->getEntryKind() == MachineJumpTableInfo::EK_LabelDifference32 && | ||
if (MJTI.getEntryKind() == MachineJumpTableInfo::EK_LabelDifference32 && | ||
MAI->doesSetDirectiveSuppressReloc()) { | ||
Value = MCSymbolRefExpr::create(GetJTSetSymbol(UID, MBB->getNumber()), | ||
OutContext); | ||
|
@@ -3051,7 +3109,7 @@ void AsmPrinter::emitJumpTableEntry(const MachineJumpTableInfo *MJTI, | |
|
||
assert(Value && "Unknown entry kind!"); | ||
|
||
unsigned EntrySize = MJTI->getEntrySize(getDataLayout()); | ||
unsigned EntrySize = MJTI.getEntrySize(getDataLayout()); | ||
OutStreamer->emitValue(Value, EntrySize); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the attempt to reuse this vector for both cases - with and without partitioning makes the code more complex than it needs to be. Consider the following refactoring:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.