-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[CodeGen] Introduce Static Data Splitter pass #122183
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
Changes from 2 commits
5d207e9
34b6b9b
1bacc51
8a85d1a
e54dacb
366bbbf
c8c122c
e5e0cee
89c80ad
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 |
---|---|---|
|
@@ -28,15 +28,17 @@ namespace llvm { | |
class MachineBasicBlock; | ||
class DataLayout; | ||
class raw_ostream; | ||
enum class DataHotness; | ||
|
||
/// MachineJumpTableEntry - One jump table in the jump table info. | ||
/// | ||
struct MachineJumpTableEntry { | ||
/// MBBs - The vector of basic blocks from which to create the jump table. | ||
std::vector<MachineBasicBlock*> MBBs; | ||
|
||
explicit MachineJumpTableEntry(const std::vector<MachineBasicBlock*> &M) | ||
: MBBs(M) {} | ||
DataHotness Hotness; | ||
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. Add a comment for the new member. 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. done, and described its usage in the comment. |
||
|
||
explicit MachineJumpTableEntry(const std::vector<MachineBasicBlock *> &M); | ||
}; | ||
|
||
class MachineJumpTableInfo { | ||
|
@@ -107,6 +109,8 @@ class MachineJumpTableInfo { | |
return JumpTables; | ||
} | ||
|
||
void updateJumpTableHotness(size_t JTI, DataHotness Hotness); | ||
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. Add a comment explaining this is for the jump table static data object? 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. Added a comment for this method, renamed it to
It makes sense to comment about how the information is used. The comment in |
||
|
||
/// RemoveJumpTable - Mark the specific index as being dead. This will | ||
/// prevent it from being emitted. | ||
void RemoveJumpTable(unsigned Idx) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,6 +71,10 @@ namespace llvm { | |
/// using profile information. | ||
MachineFunctionPass *createMachineFunctionSplitterPass(); | ||
|
||
/// createStaticDataSplitterPass - This pass partions static data sections | ||
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. typo: partitions Also should it be "partitions static data objects into hot and cold sections"? 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. Done. |
||
/// into a hot and cold section using profile information. | ||
MachineFunctionPass *createStaticDataSplitterPass(); | ||
|
||
/// MachineFunctionPrinter pass - This pass prints out the machine function to | ||
/// the given stream as a debugging tool. | ||
MachineFunctionPass * | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1291,6 +1291,10 @@ const unsigned MachineFunction::DebugOperandMemNumber = 1000000; | |
// MachineJumpTableInfo implementation | ||
//===----------------------------------------------------------------------===// | ||
|
||
MachineJumpTableEntry::MachineJumpTableEntry( | ||
const std::vector<MachineBasicBlock *> &MBBs) | ||
: MBBs(MBBs), Hotness(DataHotness::Unknown) {} | ||
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. If you initialize 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. The constructor is moved here for coding style considerations in the specific [1] code paths. The code is written in a way that [1] For CodeGen, its header directory has {MachineJumpTableInfo.h, MachineConstantPool.h. MachineFunction.h}, and |
||
|
||
/// Return the size of each entry in the jump table. | ||
unsigned MachineJumpTableInfo::getEntrySize(const DataLayout &TD) const { | ||
// The size of a jump table entry is 4 bytes unless the entry is just the | ||
|
@@ -1340,6 +1344,15 @@ unsigned MachineJumpTableInfo::createJumpTableIndex( | |
return JumpTables.size()-1; | ||
} | ||
|
||
void MachineJumpTableInfo::updateJumpTableHotness(size_t JTI, | ||
DataHotness Hotness) { | ||
ellishg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
assert(JTI < JumpTables.size() && "Invalid JTI!"); | ||
// Note record the largest hotness is important for mergable data (constant | ||
mingmingl-llvm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// pools). Even if jump table instances are not merged, record the largest | ||
// value seen fwiw. | ||
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. nit: drop 'fwiw`. 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. done. |
||
JumpTables[JTI].Hotness = std::max(JumpTables[JTI].Hotness, Hotness); | ||
} | ||
|
||
/// If Old is the target of any jump tables, update the jump tables to branch | ||
/// to New instead. | ||
bool MachineJumpTableInfo::ReplaceMBBInJumpTables(MachineBasicBlock *Old, | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,154 @@ | ||||||||||||||||||||||||||
//===- StaticDataSplitter.cpp ---------------------------------------------===// | ||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||||||||||||||||||||||||||
// See https://llvm.org/LICENSE.txt for license information. | ||||||||||||||||||||||||||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||
//===----------------------------------------------------------------------===// | ||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||
// This pass uses profile information to partition static data sections into | ||||||||||||||||||||||||||
// hot and cold ones. It begins to split jump tables based on profile, and | ||||||||||||||||||||||||||
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. Instead of "It begins... internal data." maybe have a short bullet list like "The pass uses branch profile data to assign hotness based section qualifiers for the following types of static data:
Then we can update the TODOs as we go along. 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. done. |
||||||||||||||||||||||||||
// subsequent patches will handle constant pools and other module internal data. | ||||||||||||||||||||||||||
// | ||||||||||||||||||||||||||
// For the original RFC of this pass please see | ||||||||||||||||||||||||||
// https://discourse.llvm.org/t/rfc-profile-guided-static-data-partitioning/83744. | ||||||||||||||||||||||||||
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. nit: drop the period at the end so it's easier to copy paste etc. 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. done. |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
#include "llvm/ADT/Statistic.h" | ||||||||||||||||||||||||||
#include "llvm/Analysis/ProfileSummaryInfo.h" | ||||||||||||||||||||||||||
#include "llvm/CodeGen/MBFIWrapper.h" | ||||||||||||||||||||||||||
#include "llvm/CodeGen/MachineBasicBlock.h" | ||||||||||||||||||||||||||
#include "llvm/CodeGen/MachineBlockFrequencyInfo.h" | ||||||||||||||||||||||||||
#include "llvm/CodeGen/MachineBranchProbabilityInfo.h" | ||||||||||||||||||||||||||
#include "llvm/CodeGen/MachineConstantPool.h" | ||||||||||||||||||||||||||
#include "llvm/CodeGen/MachineFunction.h" | ||||||||||||||||||||||||||
#include "llvm/CodeGen/MachineFunctionPass.h" | ||||||||||||||||||||||||||
#include "llvm/CodeGen/MachineJumpTableInfo.h" | ||||||||||||||||||||||||||
#include "llvm/CodeGen/Passes.h" | ||||||||||||||||||||||||||
#include "llvm/InitializePasses.h" | ||||||||||||||||||||||||||
#include "llvm/Pass.h" | ||||||||||||||||||||||||||
#include "llvm/Support/CommandLine.h" | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
using namespace llvm; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
#define DEBUG_TYPE "static-data-splitter" | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
STATISTIC(NumHotJumpTables, "Number of hot jump tables seen"); | ||||||||||||||||||||||||||
STATISTIC(NumColdJumpTables, "Number of cold jump tables seen"); | ||||||||||||||||||||||||||
STATISTIC(NumUnknownJumpTables, | ||||||||||||||||||||||||||
"Number of jump tables with unknown hotness. Such jump tables will " | ||||||||||||||||||||||||||
"be placed in the hot-suffixed section by default."); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
class StaticDataSplitter : public MachineFunctionPass { | ||||||||||||||||||||||||||
const MachineBranchProbabilityInfo *MBPI = nullptr; | ||||||||||||||||||||||||||
const MachineBlockFrequencyInfo *MBFI = nullptr; | ||||||||||||||||||||||||||
const ProfileSummaryInfo *PSI = nullptr; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// Returns true iff any jump table is hot-cold categorized. | ||||||||||||||||||||||||||
bool splitJumpTables(MachineFunction &MF); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// Same as above but works on functions with profile information. | ||||||||||||||||||||||||||
bool splitJumpTablesWithProfiles(MachineFunction &MF, | ||||||||||||||||||||||||||
MachineJumpTableInfo &MJTI); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
public: | ||||||||||||||||||||||||||
static char ID; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
StaticDataSplitter() : MachineFunctionPass(ID) { | ||||||||||||||||||||||||||
initializeStaticDataSplitterPass(*PassRegistry::getPassRegistry()); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
StringRef getPassName() const override { return "Static Data Splitter"; } | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
void getAnalysisUsage(AnalysisUsage &AU) const override { | ||||||||||||||||||||||||||
MachineFunctionPass::getAnalysisUsage(AU); | ||||||||||||||||||||||||||
AU.addRequired<MachineBranchProbabilityInfoWrapperPass>(); | ||||||||||||||||||||||||||
AU.addRequired<MachineBlockFrequencyInfoWrapperPass>(); | ||||||||||||||||||||||||||
AU.addRequired<ProfileSummaryInfoWrapperPass>(); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
bool runOnMachineFunction(MachineFunction &MF) override; | ||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
bool StaticDataSplitter::runOnMachineFunction(MachineFunction &MF) { | ||||||||||||||||||||||||||
MBPI = &getAnalysis<MachineBranchProbabilityInfoWrapperPass>().getMBPI(); | ||||||||||||||||||||||||||
MBFI = &getAnalysis<MachineBlockFrequencyInfoWrapperPass>().getMBFI(); | ||||||||||||||||||||||||||
PSI = &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// Split jump tables based on profile information. Subsequent patches will | ||||||||||||||||||||||||||
// handle other data types like constant pools, module-internal data, etc. | ||||||||||||||||||||||||||
return splitJumpTables(MF); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
bool StaticDataSplitter::splitJumpTablesWithProfiles( | ||||||||||||||||||||||||||
MachineFunction &MF, 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. I think MachineFunction can be const. 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. done. |
||||||||||||||||||||||||||
int NumChangedJumpTables = 0; | ||||||||||||||||||||||||||
// Regard a jump table as hot by default. If the source and all of destination | ||||||||||||||||||||||||||
// blocks are cold, regard the jump table as cold. While a destination block | ||||||||||||||||||||||||||
// does not read a jump table (unless it's also a source block), a hot | ||||||||||||||||||||||||||
// destination heuristically makes its jump table hot to accommodate for | ||||||||||||||||||||||||||
// potential profile data skews (from sampled profiles, for example). | ||||||||||||||||||||||||||
mingmingl-llvm marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
DataHotness Hotness = DataHotness::Hot; | ||||||||||||||||||||||||||
for (const auto &MBB : MF) { | ||||||||||||||||||||||||||
ellishg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
// IMPORTANT, `getJumpTableIndex` is a thin wrapper around per-target | ||||||||||||||||||||||||||
// interface `TargetInstrInfo::getjumpTableIndex`, and only X86 implements | ||||||||||||||||||||||||||
// it so far. | ||||||||||||||||||||||||||
const int JTI = MBB.getJumpTableIndex(); | ||||||||||||||||||||||||||
// This is not a source block of jump table. | ||||||||||||||||||||||||||
if (JTI == -1) | ||||||||||||||||||||||||||
continue; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
bool AllBlocksCold = true; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (!PSI->isColdBlock(&MBB, MBFI)) | ||||||||||||||||||||||||||
AllBlocksCold = false; | ||||||||||||||||||||||||||
mingmingl-llvm marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
for (const MachineBasicBlock *MBB : MJTI.getJumpTables()[JTI].MBBs) | ||||||||||||||||||||||||||
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. Is the profile count of the source BB the sum of all target BB's count? 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 expect this is the case for most cases, but a target BB can in theory end with a conditional branch to itself (as code transformation goes on). While a destination block does not read a jump table (unless it's also a source block), a hot destination heuristically makes its jump table hot to accommodate for potential profile data skews (caused by debug information loss in the sampled binary for example). I added this in the code comment. 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 received multiple feedback about the rationale to count for destination block hotness (thanks @snehasish and @weiguozhi ), and it become clearer to me the argument to make up for profile inaccuracy doesn't really hold very well. For now I updated the patch to use source block hotness only. I will do a sanity check (with and without destination block hotness) using a PGO binary, and update the data early next week. 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.
The TL,DR is that counting destination blocks' hotness on top of counting source block hotness on hot / cold jump table size (as shown by the first table below), but it doesn't change the cold/hot jump table size ratio of a binary too much (as shown by the second table) Moreover, I printed the function names in which destination block changed jump table hotness for two iFDO-optimized binaries, and manually checked a couple of such function's cycle percentage out of the whole binary. They are mostly cold functions themselves. So it's fine to not account for destination block hotness. Data size
|
||||||||||||||||||||||||||
if (!PSI->isColdBlock(MBB, MBFI)) | ||||||||||||||||||||||||||
AllBlocksCold = false; | ||||||||||||||||||||||||||
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. Is it necessary to exit quickly upon finding the hot block? 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 make sense to exit quickly for the original code. In the updated patch, only source block hotness is used. |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (AllBlocksCold) { | ||||||||||||||||||||||||||
Hotness = DataHotness::Cold; | ||||||||||||||||||||||||||
++NumColdJumpTables; | ||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||
++NumHotJumpTables; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
MF.getJumpTableInfo()->updateJumpTableHotness(JTI, Hotness); | ||||||||||||||||||||||||||
++NumChangedJumpTables; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
return NumChangedJumpTables > 0; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
bool StaticDataSplitter::splitJumpTables(MachineFunction &MF) { | ||||||||||||||||||||||||||
MachineJumpTableInfo *MJTI = MF.getJumpTableInfo(); | ||||||||||||||||||||||||||
if (!MJTI || MJTI->getJumpTables().empty()) | ||||||||||||||||||||||||||
return false; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// Place jump tables according to block hotness if function has profile data. | ||||||||||||||||||||||||||
if (PSI && PSI->hasProfileSummary() && MBFI && | ||||||||||||||||||||||||||
MF.getFunction().hasProfileData()) | ||||||||||||||||||||||||||
return splitJumpTablesWithProfiles(MF, *MJTI); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// Conservatively place all jump tables in the hot-suffixed section if profile | ||||||||||||||||||||||||||
// information for the function is not available, or the target doesn't | ||||||||||||||||||||||||||
// implement `TargetInstrInfo::getJumpTableIndex` yet. | ||||||||||||||||||||||||||
for (size_t JTI = 0; JTI < MJTI->getJumpTables().size(); JTI++) | ||||||||||||||||||||||||||
MF.getJumpTableInfo()->updateJumpTableHotness(JTI, DataHotness::Hot); | ||||||||||||||||||||||||||
ellishg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
NumUnknownJumpTables += MJTI->getJumpTables().size(); | ||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
char StaticDataSplitter::ID = 0; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
INITIALIZE_PASS_BEGIN(StaticDataSplitter, DEBUG_TYPE, "Split static data", | ||||||||||||||||||||||||||
false, false) | ||||||||||||||||||||||||||
INITIALIZE_PASS_DEPENDENCY(MachineBranchProbabilityInfoWrapperPass) | ||||||||||||||||||||||||||
INITIALIZE_PASS_DEPENDENCY(MachineBlockFrequencyInfoWrapperPass) | ||||||||||||||||||||||||||
INITIALIZE_PASS_DEPENDENCY(ProfileSummaryInfoWrapperPass) | ||||||||||||||||||||||||||
INITIALIZE_PASS_END(StaticDataSplitter, DEBUG_TYPE, "Split static data", false, | ||||||||||||||||||||||||||
false) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
MachineFunctionPass *llvm::createStaticDataSplitterPass() { | ||||||||||||||||||||||||||
return new StaticDataSplitter(); | ||||||||||||||||||||||||||
} |
Uh oh!
There was an error while loading. Please reload this page.