-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Enable hot/cold splitting of EH funclets #71236
Changes from 1 commit
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 |
---|---|---|
|
@@ -3354,6 +3354,34 @@ void Compiler::fgCreateFunclets() | |
#endif // DEBUG | ||
} | ||
|
||
/*------------------------------------------------------------------------- | ||
* | ||
* Walk the EH funclet blocks of a function to determine if the funclet | ||
* section is cold. If any of the funclets are hot, then it may not be | ||
* beneficial to split at fgFirstFuncletBB, moving all funclets to the | ||
* cold section. | ||
*/ | ||
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. Can you use the "new style" function comments? They look like
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. Sure thing; fixed. |
||
|
||
bool Compiler::fgFuncletsAreCold() | ||
{ | ||
for (BasicBlock* block = fgFirstFuncletBB; block != nullptr; block = block->bbNext) | ||
{ | ||
#if HANDLER_ENTRY_MUST_BE_IN_HOT_SECTION | ||
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 looks like we always define Does that mean we're actually never splitting off EH any regions, or am I missing something? 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. Sorry, I didn't notice 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 may not be that simple. Presumably Looking at the git blame for jit.h this define has been like this for years now. Perhaps it is no longer applicable, or perhaps it reflects some limitation in the way the jit must report EH information back to the runtime. @jkotas do you know offhand why the jit has this constraint? If not, I can do some digging. If it is indeed OK to change this, I'd recommend changing the define instead, so that the rest of the jit is freed from this constraint as well, and do that as a prerequisite, stand-alone change. 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 do not know the reason behind this. The EE side is using |
||
if (bbIsHandlerBeg(block)) | ||
{ | ||
return false; | ||
} | ||
#endif // HANDLER_ENTRY_MUST_BE_IN_HOT_SECTION | ||
|
||
if (!block->isRunRarely()) | ||
{ | ||
return false; | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
#endif // defined(FEATURE_EH_FUNCLETS) | ||
|
||
/*------------------------------------------------------------------------- | ||
|
@@ -3394,17 +3422,6 @@ PhaseStatus Compiler::fgDetermineFirstColdBlock() | |
} | ||
#endif // DEBUG | ||
|
||
#if defined(FEATURE_EH_FUNCLETS) | ||
// TODO-CQ: handle hot/cold splitting in functions with EH (including synchronized methods | ||
// that create EH in methods without explicit EH clauses). | ||
|
||
if (compHndBBtabCount > 0) | ||
{ | ||
JITDUMP("No procedure splitting will be done for this method with EH (implementation limitation)\n"); | ||
return PhaseStatus::MODIFIED_NOTHING; | ||
} | ||
#endif // FEATURE_EH_FUNCLETS | ||
|
||
BasicBlock* firstColdBlock = nullptr; | ||
BasicBlock* prevToFirstColdBlock = nullptr; | ||
BasicBlock* block; | ||
|
@@ -3413,8 +3430,8 @@ PhaseStatus Compiler::fgDetermineFirstColdBlock() | |
bool forceSplit = false; | ||
|
||
#ifdef DEBUG | ||
// If stress-splitting, split right after the first block; don't handle functions with EH | ||
forceSplit = JitConfig.JitStressProcedureSplitting() && (compHndBBtabCount == 0); | ||
// If stress-splitting, split right after the first block | ||
forceSplit = JitConfig.JitStressProcedureSplitting(); | ||
#endif | ||
|
||
if (forceSplit) | ||
|
@@ -3449,12 +3466,29 @@ PhaseStatus Compiler::fgDetermineFirstColdBlock() | |
prevToFirstColdBlock = nullptr; | ||
} | ||
} | ||
else // (firstColdBlock == NULL) | ||
else // (firstColdBlock == NULL) -- we don't have a candidate for first cold block | ||
{ | ||
// We don't have a candidate for first cold block | ||
|
||
#ifdef FEATURE_EH_FUNCLETS | ||
// | ||
// If a function has exception handling and we haven't found the first cold block yet, | ||
// consider splitting at the first funclet; do not consider splitting between funclets, | ||
// as this may break unwind info. | ||
// | ||
if (block == fgFirstFuncletBB) | ||
{ | ||
if (fgFuncletsAreCold()) | ||
{ | ||
firstColdBlock = block; | ||
prevToFirstColdBlock = lblk; | ||
} | ||
|
||
break; | ||
} | ||
#endif // FEATURE_EH_FUNCLETS | ||
|
||
// Is this a cold block? | ||
if (!blockMustBeInHotSection && (block->isRunRarely() == true)) | ||
if (!blockMustBeInHotSection && block->isRunRarely()) | ||
{ | ||
// | ||
// If the last block that was hot was a BBJ_COND | ||
|
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.
When branching to a
finally
funclet on ARM64, we emit anINS_bl_local
instruction -- previously, we could guarantee this jump would stay within the hot section, and thus would setidjShort = true
. Now that EH funclets can be moved to the cold section, we can no longer assume this, and must setidjKeepLong
based on if we're crossing hot/cold sections. Without changing this logic, we'll hit asserts inemitOutputLJ
due to the branch being too short.