Skip to content
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

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

jakobbotsch
Copy link
Member

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:

  • Make the local created for these fat pointer calls small typed like regular small normalize-on-store locals
  • Enhance fgCastNeeded to take into account the small-typedness of these locals (like IntegralRange::ForNode)

Some regressions because we remove unnecessary casts which makes this early-out in forward sub kick in:

// Finally, profitability checks.
//
// These conditions can be checked earlier in the final version to save some throughput.
// Perhaps allowing for bypass with jit stress.
//
// If fwdSubNode is an address-exposed local, forwarding it may lose optimizations due
// to GLOB_REF "poisoning" the tree. CQ analysis shows this to not be a problem with
// structs.
//
if (fwdSubNode->OperIs(GT_LCL_VAR))
{
unsigned const fwdLclNum = fwdSubNode->AsLclVarCommon()->GetLclNum();
LclVarDsc* const fwdVarDsc = lvaGetDesc(fwdLclNum);
if (!varTypeIsStruct(fwdVarDsc) && fwdVarDsc->IsAddressExposed())
{
JITDUMP(" V%02u is address exposed\n", fwdLclNum);
return false;
}
}

Fix #99798

…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`)
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 15, 2024
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @EgorBo

Diffs

@jakobbotsch jakobbotsch requested a review from EgorBo March 15, 2024 14:22
Copy link
Member

@EgorBo EgorBo left a 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);
Copy link
Member

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?

Copy link
Member Author

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.

@jakobbotsch
Copy link
Member Author

Do you know why the assert started failing recently?

It's exposed by #99265. The callee is a shared generic in this case.

@jakobbotsch jakobbotsch merged commit ad4c7c3 into dotnet:main Mar 15, 2024
129 checks passed
@jakobbotsch jakobbotsch deleted the fix-99798 branch March 15, 2024 18:19
@EgorBo
Copy link
Member

EgorBo commented Mar 15, 2024

Do you know why the assert started failing recently?

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

@jakobbotsch
Copy link
Member Author

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).

@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion failed '!"Unexpected tree op after call marked as tailcall"'
2 participants