JIT: support OSR for synchronized methods

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[];
 };
 

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);
 }

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";

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*/);

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;

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
     {

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

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
 {

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;
+    }
+}

src/tests/JIT/opt/OSR/synchronized.csproj

@@ -0,0 +1,24 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <DebugType />
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+  <PropertyGroup>
+    <CLRTestBatchPreCommands><![CDATA[
+$(CLRTestBatchPreCommands)
+set COMPlus_TieredCompilation=1
+set COMPlus_TC_QuickJitForLoops=1
+set COMPlus_TC_OnStackReplacement=1
+]]></CLRTestBatchPreCommands>
+    <BashCLRTestPreCommands><![CDATA[
+$(BashCLRTestPreCommands)
+export COMPlus_TieredCompilation=1
+export COMPlus_TC_QuickJitForLoops=1
+export COMPlus_TC_OnStackReplacement=1
+]]></BashCLRTestPreCommands>
+  </PropertyGroup>
+</Project>