-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
JIT: removed unneeded throw helper blocks #95379
Conversation
Various optimizations can happen between the time the throw helper blocks are first requested (morph) and finally used (codegen). Prune away ones that aren't needed during the stack level setter. Fixes dotnet#93948.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsVarious optimizations can happen between the time the throw helper blocks are first requested (morph) and finally used (codegen). Prune away ones that aren't needed during the stack level setter. Fixes #93948.
|
For code size this is pure goodness, but the TP impact is a bit high. Some possible mitigations:
|
Also what I'm doing likely won't work for |
src/coreclr/jit/stacklevelsetter.cpp
Outdated
#if !FEATURE_FIXED_OUT_ARGS | ||
// Set throw blocks incoming stack depth for x86. | ||
if (throwHelperBlocksUsed && !framePointerRequired) | ||
if (throwHelperBlocksUsed && node->OperMayThrow(comp)) |
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.
Since GTF_CALL
and GTF_EXCEPT
are up-to-date and overapproximations here you can optimize TP with this trick:
runtime/src/coreclr/jit/promotion.cpp
Lines 2340 to 2349 in 3805c17
if (((*use)->gtFlags & (GTF_EXCEPT | GTF_CALL)) == 0) | |
{ | |
assert(!(*use)->OperMayThrow(m_compiler)); | |
return use; | |
} | |
if (!(*use)->OperMayThrow(m_compiler)) | |
{ | |
return use; | |
} |
TP looking okayish now (Diffs), realized the stack level setter was disabled for mac arm64, so enabled it there. As part of this I will also close out #42673. Final Diffs. If we could merge all this new analysis into lower we might be able to shave off some TP, but that seems more complicated. Since this phase previously did not run for osx-arm64 we can see the overall TP hit is around 0.5% in minopts and 0.2% when optimizing. |
Should be ready for final review. FYI @dotnet/jit-contrib |
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.
Minor notes
src/coreclr/jit/compiler.h
Outdated
|
||
typedef JitHashTable<AddCodeDscKey, AddCodeDscKey, AddCodeDsc*> AddCodeDscMap; | ||
|
||
AddCodeDscMap* fgAddCodeDscMap; |
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.
Minor, but maybe you should follow the pattern set by GetBlockToEHPreds
and many similar functions, to have a:
AddCodeDscMap* GetAddCodeDscMap()
{
if (fgAddCodeDscMap == nullptr)
{
fgAddCodeDscMap = new (getAllocator(CMK_Unknown)) AddCodeDscMap(getAllocator(CMK_Unknown));
}
return fgAddCodeDscMap;
}
Instead of having the allocation within fgAddCodeRef
src/coreclr/jit/stacklevelsetter.cpp
Outdated
} | ||
else | ||
{ | ||
// assert(((node->gtFlags & GTF_CALL) != 0) || !node->OperMayThrow(comp)); |
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.
Remove or enable
SPMI failures are known bad contexts for OSX. Ignoring. |
This fixes broken CLR tests after dotnet#95379 Co-authored-by: Dong-Heon Jung <clamp03@gmail.com>
This fixes broken CLR tests after #95379 Co-authored-by: Dong-Heon Jung <clamp03@gmail.com>
Various optimizations can happen between the time the throw helper blocks are first requested (morph) and finally used (codegen). Prune away ones that aren't needed during the stack level setter.
Fixes #93948.