-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Use patchpointinfo for OSR runtime helper #123645
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
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib |
|
Can you point out an example where this happens? OSR isn't supported in optimized code, and in unoptimized code all the live state at a patchpoint should be on the stack. Sounds like register stress is doing something wrong. |
|
The issue isn't about OSR's live state, but about caller's callee-saved registers that Tier0 must preserve. Scenario: Repro: x64 doesn't hit this because its OSR epilog explicitly pops Tier0's callee-saves from stack. Register stress is behaving correctly; the fix reads callee-saves from Tier0's stack save area instead of TransitionBlock. |
|
Ah, ok. We should move these other archs to the same plan as x64 (see #33658), but that is likely easier said than done. But perhaps some of the things that made it hard before have since been fixed. I went looking for non-stressregs failures in this weekend's runs. There is an x64 failure here, though given the above it is likely something different. net11.0-windows-Release-x64-jitosr_stress_random |
|
NativeAOT size regression failure is #123667 (unrelated; failing since 19+ hours in main). @AndyAyersMS, @jakobbotsch, could you please run @AndyAyersMS, I tried running a few libs tests with on win-x64, but couldn't make it fail locally. We can open a separate issue to investigate PGO pipeline issue, I will take a look after this stress test thing is done. Having a
This PR is bringing others partially to x64 plan (JIT side is now same as it populates the patchpointinfo, helper deviates at where to reads info from TransitionBlock on x64 for both modified and unmodified registers and ppInfo on arm64 etc. for modified registers), the remaining partial difference could be resolved by putting x64 to the new plan, but either way, as you said, it's a bit tricky to consolidate. :) |
|
/azp run runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Do we have a theory on the x64 failures? @jakobbotsch and I were chatting and thought perhaps x64 was insulated... but it appears not. |
|
I just pushed a commit bringing to the same plan, lets see. I wasn't able to repro one reported issue #123671 (comment) with and without CET, but @janvorli has a private repro with diagnostics tool. |
d197992 to
3ed158b
Compare
|
This PR should simplify my job for #120865 as well. |
|
@jakobbotsch, patchpointinfo approach is now looking a bit promising (failures are unrelated; timeout / missing logs etc.), lets see what |
|
/azp run runtime-coreclr libraries-pgo, runtime-coreclr jitstressregs, runtime-coreclr jitstress2-jitstressregs, runtime-jit-experimental |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
JIT stress test failures and PGO are matching those of main (I looked through today's runs of those pipelines on main). Please let me know if you find a new relevant failure. @janvorli, could you please run internal diagnostic tool against this branch which was failing before the revert? It is now rewritten using patchpointinfo for callee-saved regs. |
On ARM64/LoongArch64/RISC-V64, OSR transitions were reading callee-saved register values from the TransitionBlock, which contains the register values at the moment JIT_Patchpoint was called. Under register stress (or normal optimization), Tier0 may reuse callee-saved registers as temporaries, so these values can be garbage rather than the preserved values the caller expects. This fix reads callee-saves from Tier0's stack save area using offset information recorded in PatchpointInfo during Tier0 compilation; the same location where Tier0 saved the caller's original values in its prolog. x64 doesn't need this because its JIT epilog explicitly pops Tier0's callee-saves from the stack, but bringing it to the same plan for consistency.
Closes #123608
Closes #123605