-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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: Type fat pointer locals precisely, and avoid unnecessary normalization in inlining #99806
Conversation
…zation in inlining During call importation, for fat pointer calls we will introduce a local and spill the call to it. This loses track of the small typedness of the value, which can cause inlining to introduce unnecessary normalization casts later. For tailcalls this can cause us to add IR after the call that we do not expect, causing issues like dotnet#99798. Fix the problem by enhancing logic in a few places: - Make the local created for these fat pointer calls small typed like regular normalize-on-store locals - Enhance `fgCastNeeded` to take into account the small-typedness of these locals (like `IntegralRange::ForNode`)
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
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.
Do you know why the assert started failing recently?
|
||
// TODO-Bug: CHECK_SPILL_NONE here looks wrong. | ||
impStoreTemp(calliSlot, call, CHECK_SPILL_NONE); |
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.
why not fix it then?
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.
I opened #99808 about fixing it. I mainly think we should separate the diffs + create a regression test for it.
It's exposed by #99265. The callee is a shared generic in this case. |
Weird, I had many CI runs in my PR and didn't see it there, but it's possible of course |
It did fail in the runtime-nativeaot-outerloop run on that PR: https://dev.azure.com/dnceng-public/public/_build/results?buildId=596527&view=results (you can see the assert in the console log for "windows-x64 Checked NativeAOT_Pri0" there). |
During call importation, for fat pointer calls we will introduce a local and spill the call to it. This loses track of the small typedness of the value, which can cause inlining to introduce unnecessary normalization casts later. For tailcalls this can cause us to add IR after the call that we do not expect, causing issues like #99798.
Fix the problem by enhancing logic in a few places:
fgCastNeeded
to take into account the small-typedness of these locals (likeIntegralRange::ForNode
)Some regressions because we remove unnecessary casts which makes this early-out in forward sub kick in:
runtime/src/coreclr/jit/forwardsub.cpp
Lines 724 to 743 in b94364e
Fix #99798