JIT: support OSR for synchronized methods #61712
Conversation
OSR methods share the "monitor acquired" flag with the original method. The monitor acquired flag is s bit of non-IL live state that must be tecorded in the patchpoint. Also, OSR methods only need to release the monitor as acquisition can only happen in the original method.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsOSR methods share the "monitor acquired" flag with the original method. Also, OSR methods only need to release the monitor as acquisition can
|
|
@dotnet/jit-contrib PTAL |
|
|
||
| for (int i = 0; i < 100_000; i++) | ||
| { | ||
| s = z.F(); |
There was a problem hiding this comment.
Does it make sense to verify Monitor.IsEntered(this) inside the loop?
There was a problem hiding this comment.
Sure, will add this.
| if (result == 100) { | ||
| Console.WriteLine("SUCCESS"); | ||
| } | ||
| else { | ||
| Console.WriteLine("FAILURE"); | ||
| } |
There was a problem hiding this comment.
Nit:
| if (result == 100) { | |
| Console.WriteLine("SUCCESS"); | |
| } | |
| else { | |
| Console.WriteLine("FAILURE"); | |
| } | |
| if (result == 100) | |
| { | |
| Console.WriteLine("SUCCESS"); | |
| } | |
| else | |
| { | |
| Console.WriteLine("FAILURE"); | |
| } |
|
/azp run runtime-jit-experimental |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| BasicBlock* tryBegBB = fgNewBBafter(BBJ_NONE, fgFirstBB, false); | ||
| BasicBlock* tryNextBB = tryBegBB->bbNext; | ||
| BasicBlock* tryLastBB = fgLastBB; | ||
| BasicBlock* const tryBegBB = fgSplitBlockAtEnd(fgFirstBB); |
There was a problem hiding this comment.
Why was the switch from fgNewBBafter to fgSplitBlockAtEnd required?
There was a problem hiding this comment.
Because in OSR methods fgFirstBB may not be BBJ_NONE.
There was a problem hiding this comment.
Even after calling fgEnsureFirstBBisScratch()?
There was a problem hiding this comment.
Yes, because fgEnsureFirstBBisScratch will just return if the first bb is already scratch.
For OSR we've already called this and messed with the resulting flow. See fgFixEntryFlowForOSR.
Right, not yet enabled anywhere but x64. Arm64 support is next, then maybe I'll look at what we'd need to do for x86. |
|
Seeing a few unexpected OSR stress issues. Will need to investigate. |
|
Bug was that if you managed to remove all the EH table entries we'd null out the EH table. Then when we'd go to add a new entry for the synchronous try/fault we don't realize the table is gone and the jit goes boom. We could probably hit this with the right test case even without OSR (only dead EH in a synchronous method). |
|
/azp run runtime-jit-experimental |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Back down to the "expected" set of failures, with one exception in osr + pc (likely unrelated). I'll track this one separately. |
|
@BruceForstall want to take one last look? I had to make a change in jiteh.cpp to fix this issue. |
Looks like we only ever removed EH clauses after the code to add sync method EH, so it wouldn't hit before. The jiteh.cpp change makes sense: there's no reason to remove the table if |
ghost
locked as resolved and limited conversation to collaborators
OSR methods share the "monitor acquired" flag with the original method.
The monitor acquired flag is s bit of non-IL live state that must be
tecorded in the patchpoint.
Also, OSR methods only need to release the monitor as acquisition can
only happen in the original method.