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: revise approach for x64 OSR epilogs #65609

Merged
merged 7 commits into from
Feb 21, 2022

Conversation

AndyAyersMS
Copy link
Member

Rework x64 OSR so OSR methods have standard epilogs.

Details in attached doc.

Rework x64 OSR so OSR methods have standard epilogs.

Details in attached doc.
@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 Feb 19, 2022
@ghost ghost assigned AndyAyersMS Feb 19, 2022
@ghost
Copy link

ghost commented Feb 19, 2022

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

Issue Details

Rework x64 OSR so OSR methods have standard epilogs.

Details in attached doc.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

@BruceForstall PTAL

cc @dotnet/jit-contrib

Should see diffs in x64 asp.net for Tier0 + patchpoint and OSR methods, but not any other collection.

@AndyAyersMS
Copy link
Member Author

SysV failing because RBM_INT_CALLEE_SAVED does not normally include RBP -- need to review how I use this.

@AndyAyersMS
Copy link
Member Author

Diffs are as expected.

The bigger regressions are in Tier0 methods with patchpoints, because the enlarged callee save area leads to larger offsets to locals.

Top method regressions (bytes):
         270 (12.06 % of base) : 7222.dasm - JsonWriterHelper:ToUtf8(ReadOnlySpan`1,Span`1,byref,byref):int
         263 (9.24 % of base) : 1496.dasm - JsonWriterHelper:ToUtf8(ReadOnlySpan`1,Span`1,byref,byref):int
         260 (7.12 % of base) : 1316.dasm - Utf8Utility:GetPointerToFirstInvalidByte(long,int,byref,byref):long

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Feb 19, 2022

Going to run some extra legs: jit-experimental and libraries pgo. Neither of them is currently clean so will have to interpret the results.

For jit-experimental the only failures are on arm64. Since this change is x64 only, we should see just those failures.

In current libraries pgo we see failures with OSR enabled:

Assert failure(PID 3960 [0x00000f78], Thread: 2708 [0x0a94]): ((FARPROC) (TADDR)m_pvHJRetAddr) != NULL

which are hallmarks of bad epilog unwinds. Those should be gone with this fix.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Feb 19, 2022

Looks like the changes are causing issues for some OSR cases on arm64. Will investigate.

Also, EH tests on Linux x64 not happy.

Comment on lines +9139 to +9148
for (regNumber reg = REG_INT_LAST; tier0IntCalleeSaves != RBM_NONE; reg = REG_PREV(reg))
{
regMaskTP regBit = genRegMask(reg);

if ((regBit & tier0IntCalleeSaves) != 0)
{
compiler->unwindPush(reg);
}
tier0IntCalleeSaves &= ~regBit;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe consider using genFindLowestReg here? Took me a bit to understand this loop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working backwards through the "preferred pop order" for registers. It needs to match what we do in epilog gen (where we walk from REG_INT_FIRST via REG_NEXT.

I don't know if that corresponds to numeric order.

I also don't know if the order actually matters for x64 (as long as prolog, epilog, and unwind agree) -- I think the ordering requirement may be an x86-ism where the code manager does a lot of inspection of the jitted codegen rather than relying on unwind. In particular for x64 OSR there can now be two disjoint ranges of pushes/pops; each range will be in the proper order but overall the registers may be handled out of order.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. That makes sense.

src/coreclr/jit/codegenxarch.cpp Show resolved Hide resolved
@AndyAyersMS
Copy link
Member Author

Hopefully that's the last of the missing bits of code.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

With the most recent fix, I expect jit-experimental to be clean. We'll see.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

With the most recent fix, I expect jit-experimental to be clean. We'll see.

Almost. One pre-existing failure not yet fixed.

"D:\h\w\B5BB0A3A\p\corerun.exe" -p "System.Reflection.Metadata.MetadataUpdater.IsSupported=false"  GitHub_21990.dll
Expected: 100
Actual: -1073741819

@AndyAyersMS AndyAyersMS merged commit 537d607 into dotnet:main Feb 21, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2022
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.

2 participants