[BOLT] Add structure of CDSplit to SplitFunctions #73430
Conversation
|
A test will be added in a later PR with the complete CDSplit logic. |
This commit establishes the general structure of the CDSplit strategy in SplitFunctions without incorporating the exact splitting logic. With -split-functions -split-strategy=cdsplit, the SplitFunctions pass will run twice: the first time is before function reordering and functions are hot-cold split; the second time is after function reordering and functions are hot-warm-cold split based on the fixed function ordering. Currently, all functions are hot-warm split after the entry block in the second splitting pass. Subsequent commits will introduce the precise splitting logic.
ShatianWang
force-pushed
the
112523Commit2/3_SetupCDSplitStructure
branch
from
4f1d6e9
to
e9e9c2d
Compare
bolt/lib/Passes/SplitFunctions.cpp
Outdated
| @@ -136,6 +149,55 @@ struct SplitProfile2 final : public SplitStrategy { | |||
| } | |||
| }; | |||
|
|
|||
| struct SplitCacheDirected final : public SplitStrategy { | |||
| BinaryContext &BC; | |||
There was a problem hiding this comment.
Do we need BinaryContext member? We can always access it via BinaryFunction::getBinaryContext().
bolt/lib/Passes/SplitFunctions.cpp
Outdated
| return BF.hasValidProfile() && hasFullProfile(BF) && !allBlocksCold(BF); | ||
| } | ||
|
|
||
| bool keepEmpty() override { return true; } |
There was a problem hiding this comment.
Could you please add a comment on why we need keepEmpty() for this strategy?
There was a problem hiding this comment.
bool compactFragments() override { return false; }?
bolt/lib/Passes/SplitFunctions.cpp
Outdated
| @@ -409,8 +487,10 @@ void SplitFunctions::splitFunction(BinaryFunction &BF, SplitStrategy &S) { | |||
| LLVM_DEBUG(dbgs() << "Estimated size for function " << BF | |||
| << " post-split is <0x" << Twine::utohexstr(HotSize) | |||
| << ", 0x" << Twine::utohexstr(ColdSize) << ">\n"); | |||
| if (alignTo(OriginalHotSize, opts::SplitAlignThreshold) <= | |||
| alignTo(HotSize, opts::SplitAlignThreshold) + opts::SplitThreshold) { | |||
| if (S.autoReversal() && | |||
There was a problem hiding this comment.
Do you mean to run calculateEmittedSize() above even when autoReversal() is off?
There was a problem hiding this comment.
Removed autoReversal() per our offline discussion.
|
Based on comments from and offline discussion with @maksfb, added fixup! [BOLT] Add structure of CDSplit to SplitFunctions |
| @@ -430,6 +430,8 @@ void BinaryFunctionPassManager::runAllPasses(BinaryContext &BC) { | |||
| Manager.registerPass( | |||
| std::make_unique<ReorderFunctions>(PrintReorderedFunctions)); | |||
|
|
|||
| Manager.registerPass(std::make_unique<SplitFunctions>(PrintSplit)); | |||
There was a problem hiding this comment.
Please add a brief comment why we need another pass of SplitFunctions here too.
bolt/lib/Passes/SplitFunctions.cpp
Outdated
| return BF.hasValidProfile() && hasFullProfile(BF) && !allBlocksCold(BF); | ||
| } | ||
|
|
||
| bool keepEmpty() override { return true; } |
There was a problem hiding this comment.
bool compactFragments() override { return false; }?
|
New fixup addressing the new comments by @maksfb . |
This commit establishes the general structure of the CDSplit strategy in SplitFunctions without incorporating the exact splitting logic. With -split-functions -split-strategy=cdsplit, the SplitFunctions pass will run twice: the first time is before function reordering and functions are hot-cold split; the second time is after function reordering and functions are hot-warm-cold split based on the fixed function ordering. Currently, all functions are hot-warm split after the entry block in the second splitting pass. Subsequent commits will introduce the precise splitting logic. NFC.