-
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
[Linux-arm] Do not add resolution move in BBCallAlwaysPairTail #48828
Conversation
Sample failures: https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-47307-head-79786b731dd04634a8/PayloadGroup0/console.9e154ef8.log?sv=2019-07-07&se=2021-03-03T09%3A49%3A11Z&sr=c&sp=rl&sig=rHImoZWkkCsi35nI3DzfxPkbCPTbiOg2B8uspiWOIVE%3D https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-47307-head-79786b731dd04634a8/PayloadGroup0/console.9e154ef8.log?sv=2019-07-07&se=2021-03-03T09%3A49%3A11Z&sr=c&sp=rl&sig=rHImoZWkkCsi35nI3DzfxPkbCPTbiOg2B8uspiWOIVE%3D
@dotnet/jit-contrib |
src/coreclr/jit/lsra.cpp
Outdated
// In such case, mark that we do not want to insert resolution moves in it. | ||
if (block->bbPreds == nullptr && block->isBBCallAlwaysPairTail()) | ||
{ | ||
blockInfo[block->bbNum].hasEHBoundaryIn = true; |
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.
You might note this is a bit of a subterfuge, as this block has no flow whatsoever.
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.
Yes, agree and since we never had a flow, we were not entering the for loop above. However, thought it would be good to have the logic (that is inside for loop for isBBCallAlwaysPairTail()
) outside the loop for no flow scenarios.
src/coreclr/jit/lsra.cpp
Outdated
@@ -887,6 +887,17 @@ void LinearScan::setBlockSequence() | |||
} | |||
} | |||
|
|||
#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) |
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.
This code seems odd to me, since we're already handling the callfinally/always pair stuff in the loop above, for all platforms. But the code above is weird because the condition is loop-invariant. It seems like we should just hoist:
// We treat BBCallAlwaysPairTail blocks as having EH flow, since we can't
// insert resolution moves into those blocks.
if (block->isBBCallAlwaysPairTail())
{
blockInfo[block->bbNum].hasEHBoundaryIn = true;
blockInfo[block->bbNum].hasEHBoundaryOut = true;
}
above the pred
loop (for all platforms), and then replace it in the loop with:
if (!block->isBBCallAlwaysPairTail())
...
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.
That's a good point. Fixed.
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.
LGTM
We were adding resolution move in ALWAYS part of
BBJ_CALLFINALLY/BBJ_ALWAYS
pair which should ideally be empty for Arm.Fix by identifying such blocks and mark them to not do resolution in it.
Fixes following failures:
https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-47307-head-79786b731dd04634a8/PayloadGroup0/console.9e154ef8.log?sv=2019-07-07&se=2021-03-03T09%3A49%3A11Z&sr=c&sp=rl&sig=rHImoZWkkCsi35nI3DzfxPkbCPTbiOg2B8uspiWOIVE%3D
https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-47307-head-79786b731dd04634a8/PayloadGroup0/console.9e154ef8.log?sv=2019-07-07&se=2021-03-03T09%3A49%3A11Z&sr=c&sp=rl&sig=rHImoZWkkCsi35nI3DzfxPkbCPTbiOg2B8uspiWOIVE%3D