diff --git a/src/coreclr/inc/patchpointinfo.h b/src/coreclr/inc/patchpointinfo.h index 1445d8f79bce9c..d6bd2b6aee9a41 100644 --- a/src/coreclr/inc/patchpointinfo.h +++ b/src/coreclr/inc/patchpointinfo.h @@ -40,6 +40,7 @@ struct PatchpointInfo m_genericContextArgOffset = -1; m_keptAliveThisOffset = -1; m_securityCookieOffset = -1; + m_monitorAcquiredOffset = -1; } // Total size of this patchpoint info record, in bytes @@ -108,6 +109,22 @@ struct PatchpointInfo m_securityCookieOffset = offset; } + // Original method FP relative offset for monitor acquired flag + int MonitorAcquiredOffset() const + { + return m_monitorAcquiredOffset; + } + + bool HasMonitorAcquired() const + { + return m_monitorAcquiredOffset != -1; + } + + void SetMonitorAcquiredOffset(int offset) + { + m_monitorAcquiredOffset = offset; + } + // True if this local was address exposed in the original method bool IsExposed(unsigned localNum) const { @@ -141,6 +158,7 @@ struct PatchpointInfo int m_genericContextArgOffset; int m_keptAliveThisOffset; int m_securityCookieOffset; + int m_monitorAcquiredOffset; int m_offsetAndExposureData[]; }; diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 332ef138d78912..b5c52e81490e72 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5285,6 +5285,14 @@ void Compiler::generatePatchpointInfo() patchpointInfo->SecurityCookieOffset()); } + if (lvaMonAcquired != BAD_VAR_NUM) + { + LclVarDsc* const varDsc = lvaGetDesc(lvaMonAcquired); + patchpointInfo->SetMonitorAcquiredOffset(varDsc->GetStackOffset()); + JITDUMP("--OSR-- monitor acquired V%02u offset is FP %d\n", lvaMonAcquired, + patchpointInfo->MonitorAcquiredOffset()); + } + // Register this with the runtime. info.compCompHnd->setPatchpointInfo(patchpointInfo); } diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 7af00a37eee03c..0fdcc26810a20e 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4736,10 +4736,6 @@ inline bool Compiler::compCanHavePatchpoints(const char** reason) { whyNot = "localloc"; } - else if ((info.compFlags & CORINFO_FLG_SYNCH) != 0) - { - whyNot = "synchronized method"; - } else if (opts.IsReversePInvoke()) { whyNot = "reverse pinvoke"; diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 59b0ec7f939155..654683fbba3351 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -1683,12 +1683,9 @@ void Compiler::fgAddSyncMethodEnterExit() // Create a block for the start of the try region, where the monitor enter call // will go. - - assert(fgFirstBB->bbFallsThrough()); - - BasicBlock* tryBegBB = fgNewBBafter(BBJ_NONE, fgFirstBB, false); - BasicBlock* tryNextBB = tryBegBB->bbNext; - BasicBlock* tryLastBB = fgLastBB; + BasicBlock* const tryBegBB = fgSplitBlockAtEnd(fgFirstBB); + BasicBlock* const tryNextBB = tryBegBB->bbNext; + BasicBlock* const tryLastBB = fgLastBB; // If we have profile data the new block will inherit the next block's weight if (tryNextBB->hasProfileWeight()) @@ -1799,10 +1796,10 @@ void Compiler::fgAddSyncMethodEnterExit() lvaTable[lvaMonAcquired].lvType = typeMonAcquired; - { // Scope the variables of the variable initialization - - // Initialize the 'acquired' boolean. - + // Create IR to initialize the 'acquired' boolean. + // + if (!opts.IsOSR()) + { GenTree* zero = gtNewZeroConNode(genActualType(typeMonAcquired)); GenTree* varNode = gtNewLclvNode(lvaMonAcquired, typeMonAcquired); GenTree* initNode = gtNewAssignNode(varNode, zero); @@ -1825,7 +1822,7 @@ void Compiler::fgAddSyncMethodEnterExit() unsigned lvaCopyThis = 0; if (!info.compIsStatic) { - lvaCopyThis = lvaGrabTemp(true DEBUGARG("Synchronized method monitor acquired boolean")); + lvaCopyThis = lvaGrabTemp(true DEBUGARG("Synchronized method copy of this for handler")); lvaTable[lvaCopyThis].lvType = TYP_REF; GenTree* thisNode = gtNewLclvNode(info.compThisArg, TYP_REF); @@ -1835,7 +1832,12 @@ void Compiler::fgAddSyncMethodEnterExit() fgNewStmtAtEnd(tryBegBB, initNode); } - fgCreateMonitorTree(lvaMonAcquired, info.compThisArg, tryBegBB, true /*enter*/); + // For OSR, we do not need the enter tree as the monitor is acquired by the original method. + // + if (!opts.IsOSR()) + { + fgCreateMonitorTree(lvaMonAcquired, info.compThisArg, tryBegBB, true /*enter*/); + } // exceptional case fgCreateMonitorTree(lvaMonAcquired, lvaCopyThis, faultBB, false /*exit*/); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 09232cceeff737..8f6e4670b532e3 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -11644,7 +11644,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) block->bbFlags |= BBF_PARTIAL_COMPILATION_PATCHPOINT; setMethodHasPartialCompilationPatchpoint(); - // Change block to BBJ_THROW so we won't trigger importation of sucessors. + // Change block to BBJ_THROW so we won't trigger importation of successors. // block->bbJumpKind = BBJ_THROW; @@ -18855,7 +18855,7 @@ unsigned Compiler::impGetSpillTmpBase(BasicBlock* block) SetSpillTempsBase callback(baseTmp); // We do *NOT* need to reset the SpillClique*Members because a block can only be the predecessor - // to one spill clique, and similarly can only be the sucessor to one spill clique + // to one spill clique, and similarly can only be the successor to one spill clique impWalkSpillCliqueFromPred(block, &callback); return baseTmp; diff --git a/src/coreclr/jit/jiteh.cpp b/src/coreclr/jit/jiteh.cpp index 12e91d077690ba..c0fd2bf9581ce3 100644 --- a/src/coreclr/jit/jiteh.cpp +++ b/src/coreclr/jit/jiteh.cpp @@ -1405,7 +1405,12 @@ void Compiler::fgRemoveEHTableEntry(unsigned XTnum) if (compHndBBtabCount == 0) { // No more entries remaining. - compHndBBtab = nullptr; + // + // We used to null out compHndBBtab here, but with OSR + Synch method + // we may remove all the initial EH entries if not reachable in the + // OSR portion, then need to add one for the synchronous exit. + // + // So now we just leave it be. } else { diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 0c865355afe1e8..070dc76486ad7b 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -6259,10 +6259,27 @@ void Compiler::lvaAssignVirtualFrameOffsetsToLocals() if (lvaMonAcquired != BAD_VAR_NUM) { - // This var must go first, in what is called the 'frame header' for EnC so that it is - // preserved when remapping occurs. See vm\eetwain.cpp for detailed comment specifying frame - // layout requirements for EnC to work. - stkOffs = lvaAllocLocalAndSetVirtualOffset(lvaMonAcquired, lvaLclSize(lvaMonAcquired), stkOffs); + // For OSR we use the flag set up by the original method. + // + if (opts.IsOSR()) + { + assert(info.compPatchpointInfo->HasMonitorAcquired()); + int originalOffset = info.compPatchpointInfo->MonitorAcquiredOffset(); + int offset = originalFrameStkOffs + originalOffset; + + JITDUMP("---OSR--- V%02u (on old frame, monitor aquired) old rbp offset %d old frame offset %d new " + "virt offset %d\n", + lvaMonAcquired, originalOffset, originalFrameStkOffs, offset); + + lvaTable[lvaMonAcquired].SetStackOffset(offset); + } + else + { + // This var must go first, in what is called the 'frame header' for EnC so that it is + // preserved when remapping occurs. See vm\eetwain.cpp for detailed comment specifying frame + // layout requirements for EnC to work. + stkOffs = lvaAllocLocalAndSetVirtualOffset(lvaMonAcquired, lvaLclSize(lvaMonAcquired), stkOffs); + } } #ifdef JIT32_GCENCODER diff --git a/src/coreclr/jit/patchpoint.cpp b/src/coreclr/jit/patchpoint.cpp index dd250c8607a114..15a37150326183 100644 --- a/src/coreclr/jit/patchpoint.cpp +++ b/src/coreclr/jit/patchpoint.cpp @@ -26,7 +26,6 @@ // // * no patchpoints in handler regions // * no patchpoints for localloc methods -// * no patchpoints for synchronized methods (workaround) // class PatchpointTransformer { diff --git a/src/tests/JIT/opt/OSR/synchronized.cs b/src/tests/JIT/opt/OSR/synchronized.cs new file mode 100644 index 00000000000000..4a0b8629d49aa2 --- /dev/null +++ b/src/tests/JIT/opt/OSR/synchronized.cs @@ -0,0 +1,83 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using System.Threading; + +struct S +{ + public long y; + public int x; +} + +class Z +{ + virtual public S F() + { + S s = new S(); + s.x = 100; + s.y = -1; + return s; + } +} + +class X +{ + Z z; + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.Synchronized)] + public S G() + { + S s = new S(); + + for (int i = 0; i < 100_000; i++) + { + if (!Monitor.IsEntered(this)) + { + throw new Exception(); + } + s = z.F(); + } + + return s; + } + + public static int Main() + { + int result = -1; + try + { + result = Test(); + } + catch (Exception) + { + Console.WriteLine("EXCEPTION"); + } + + if (result == 100) + { + Console.WriteLine("SUCCESS"); + } + else + { + Console.WriteLine("FAILURE"); + } + return result; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static int Test() + { + var x = new X(); + x.z = new Z(); + + for (int i = 0; i < 100; i++) + { + _ = x.G(); + Thread.Sleep(15); + } + + return x.G().x; + } +} diff --git a/src/tests/JIT/opt/OSR/synchronized.csproj b/src/tests/JIT/opt/OSR/synchronized.csproj new file mode 100644 index 00000000000000..9620f75474a935 --- /dev/null +++ b/src/tests/JIT/opt/OSR/synchronized.csproj @@ -0,0 +1,24 @@ + + + Exe + + True + + + + + + + + +