JIT: yet another OSR stress mode

eng/pipelines/common/templates/runtimes/run-test-job.yml

@@ -546,6 +546,7 @@ jobs:
           - jitosr
           - jitosr_stress
           - jitosr_pgo
+          - jitosr_stress_random
           - jitpartialcompilation
           - jitpartialcompilation_osr
           - jitpartialcompilation_osr_pgo

src/coreclr/jit/codegencommon.cpp

@@ -6235,15 +6235,9 @@ void CodeGen::genEnregisterOSRArgsAndLocals()
         // This local was part of the live tier0 state and is enregistered in the
         // OSR method. Initialize the register from the right frame slot.
         //
-        // We currently don't expect to see enregistered multi-reg args in OSR methods,
-        // as struct promotion is disabled. So any struct arg just uses the location
-        // on the tier0 frame.
-        //
         // If we ever enable promotion we'll need to generalize what follows to copy each
         // field from the tier0 frame to its OSR home.
         //
-        assert(!varDsc->lvIsMultiRegArg);
-
         if (!VarSetOps::IsMember(compiler, compiler->fgFirstBB->bbLiveIn, varDsc->lvVarIndex))
         {
             // This arg or local is not live at entry to the OSR method.
@@ -7542,6 +7536,16 @@ void CodeGen::genFnProlog()
 
 #endif // PROFILING_SUPPORTED
 
+    // For OSR we may have a zero-length prolog. That's not supported
+    // when the method must report a generics context,/ so add a nop if so.
+    //
+    if (compiler->opts.IsOSR() && (GetEmitter()->emitGetPrologOffsetEstimate() == 0) &&
+        (compiler->lvaReportParamTypeArg() || compiler->lvaKeepAliveAndReportThis()))
+    {
+        JITDUMP("OSR: prolog was zero length and has generic context to report: adding nop to pad prolog.\n");
+        instGen(INS_nop);
+    }
+
     if (!GetInterruptible())
     {
         // The 'real' prolog ends here for non-interruptible methods.

src/coreclr/jit/compiler.cpp

@@ -2786,15 +2786,19 @@ void Compiler::compInitOptions(JitFlags* jitFlags)
         verboseDump = (JitConfig.JitDumpTier0() > 0);
     }
 
-    // Optionally suppress dumping some OSR jit requests.
+    // Optionally suppress dumping except for a specific OSR jit request.
     //
-    if (verboseDump && jitFlags->IsSet(JitFlags::JIT_FLAG_OSR))
-    {
-        const int desiredOffset = JitConfig.JitDumpAtOSROffset();
+    const int dumpAtOSROffset = JitConfig.JitDumpAtOSROffset();
 
-        if (desiredOffset != -1)
+    if (verboseDump && (dumpAtOSROffset != -1))
+    {
+        if (jitFlags->IsSet(JitFlags::JIT_FLAG_OSR))
+        {
+            verboseDump = (((IL_OFFSET)dumpAtOSROffset) == info.compILEntry);
+        }
+        else
         {
-            verboseDump = (((IL_OFFSET)desiredOffset) == info.compILEntry);
+            verboseDump = false;
         }
     }
 

src/coreclr/jit/fgdiagnostic.cpp

@@ -2151,11 +2151,11 @@ void Compiler::fgTableDispBasicBlock(BasicBlock* block, int ibcColWidth /* = 0 *
     {
         if (block == fgEntryBB)
         {
-            printf("original-entry");
+            printf(" original-entry");
         }
         if (block == fgOSREntryBB)
         {
-            printf("osr-entry");
+            printf(" osr-entry");
         }
     }
 

src/coreclr/jit/fgprofile.cpp

@@ -1721,12 +1721,23 @@ PhaseStatus Compiler::fgPrepareToInstrumentMethod()
     // jitting on PGO. If we ever implement a broader pattern of deferral -- say deferring
     // based on static PGO -- we will need to reconsider.
     //
+    // Under OSR stress we may add patchpoints even without backedges. So we also
+    // need to change the PGO instrumetation approach if OSR stress is enabled.
+    //
     CLANG_FORMAT_COMMENT_ANCHOR;
 
+#if defined(DEBUG)
+    const bool mayHaveStressPatchpoints =
+        (JitConfig.JitOffsetOnStackReplacement() >= 0) || (JitConfig.JitRandomOnStackReplacement() > 0);
+#else
+    const bool mayHaveStressPatchpoints = false;
+#endif
+
+    const bool mayHavePatchpoints =
+        (JitConfig.TC_OnStackReplacement() > 0) && (compHasBackwardJump || mayHaveStressPatchpoints);
     const bool prejit               = opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT);
-    const bool tier0WithPatchpoints = opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) &&
-                                      (JitConfig.TC_OnStackReplacement() > 0) && compHasBackwardJump;
-    const bool osrMethod       = opts.IsOSR();
+    const bool tier0WithPatchpoints = opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) && mayHavePatchpoints;
+    const bool osrMethod            = opts.IsOSR();
     const bool useEdgeProfiles = (JitConfig.JitEdgeProfiling() > 0) && !prejit && !tier0WithPatchpoints && !osrMethod;
 
     if (useEdgeProfiles)

src/coreclr/jit/importer.cpp

@@ -11792,28 +11792,30 @@ void Compiler::impImportBlockCode(BasicBlock* block)
 
 #ifdef FEATURE_ON_STACK_REPLACEMENT
 
-    // Are there any places in the method where we might add a patchpoint?
+    // Is OSR enabled?
     //
-    if (compHasBackwardJump)
+    if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) && (JitConfig.TC_OnStackReplacement() > 0))
     {
-        // Is OSR enabled?
+        // We don't inline at Tier0, if we do, we may need rethink our approach.
+        // Could probably support inlines that don't introduce flow.
         //
-        if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) && (JitConfig.TC_OnStackReplacement() > 0))
+        assert(!compIsForInlining());
+
+        // OSR is not yet supported for methods with explicit tail calls.
+        //
+        // But we also do not have to switch these methods to be optimized as we should be
+        // able to avoid getting trapped in Tier0 code by normal call counting.
+        // So instead, just suppress adding patchpoints.
+        //
+        if (!compTailPrefixSeen)
         {
-            // OSR is not yet supported for methods with explicit tail calls.
-            //
-            // But we also may not switch methods to be optimized as we should be
-            // able to avoid getting trapped in Tier0 code by normal call counting.
-            // So instead, just suppress adding patchpoints.
+            assert(compCanHavePatchpoints());
+
+            // The normaly policy is only to add patchpoints to the targets of lexically
+            // backwards branches.
             //
-            if (!compTailPrefixSeen)
+            if (compHasBackwardJump)
             {
-                assert(compCanHavePatchpoints());
-
-                // We don't inline at Tier0, if we do, we may need rethink our approach.
-                // Could probably support inlines that don't introduce flow.
-                assert(!compIsForInlining());
-
                 // Is the start of this block a suitable patchpoint?
                 //
                 if (((block->bbFlags & BBF_BACKWARD_JUMP_TARGET) != 0) && (verCurrentState.esStackDepth == 0))
@@ -11826,12 +11828,64 @@ void Compiler::impImportBlockCode(BasicBlock* block)
                     setMethodHasPatchpoint();
                 }
             }
+            else
+            {
+                // Should not see backward branch targets w/o backwards branches
+                assert((block->bbFlags & BBF_BACKWARD_JUMP_TARGET) == 0);
+            }
         }
-    }
-    else
-    {
-        // Should not see backward branch targets w/o backwards branches
-        assert((block->bbFlags & BBF_BACKWARD_JUMP_TARGET) == 0);
+
+#ifdef DEBUG
+        // As a stress test, we can place patchpoints at the start of any block
+        // that is a stack empty point and is not within a handler.
+        //
+        // Todo: enable for mid-block stack empty points too.
+        //
+        const int  offsetOSR    = JitConfig.JitOffsetOnStackReplacement();
+        const int  randomOSR    = JitConfig.JitRandomOnStackReplacement();
+        const bool tryOffsetOSR = offsetOSR >= 0;
+        const bool tryRandomOSR = randomOSR > 0;
+
+        if ((tryOffsetOSR || tryRandomOSR) && (verCurrentState.esStackDepth == 0) && !block->hasHndIndex() &&
+            ((block->bbFlags & BBF_PATCHPOINT) == 0))
+        {
+            // Block start can have a patchpoint. See if we should add one.
+            //
+            bool addPatchpoint = false;
+
+            // Specific offset?
+            //
+            if (tryOffsetOSR)
+            {
+                if (impCurOpcOffs == (unsigned)offsetOSR)
+                {
+                    addPatchpoint = true;
+                }
+            }
+            // Random?
+            //
+            else
+            {
+                // Reuse the random inliner's random state.
+                // Note m_inlineStrategy is always created, even if we're not inlining.
+                //
+                CLRRandom* const random      = impInlineRoot()->m_inlineStrategy->GetRandom(randomOSR);
+                const int        randomValue = (int)random->Next(100);
+
+                addPatchpoint = (randomValue < randomOSR);
+            }
+
+            if (addPatchpoint)
+            {
+                block->bbFlags |= BBF_PATCHPOINT;
+                setMethodHasPatchpoint();
+            }
+
+            JITDUMP("\n** %s patchpoint%s added to " FMT_BB " (il offset %u)\n", tryOffsetOSR ? "offset" : "random",
+                    addPatchpoint ? "" : " not", block->bbNum, impCurOpcOffs);
+        }
+
+#endif // DEBUG
     }
 
     // Mark stack-empty rare blocks to be considered for partial compilation.

src/coreclr/jit/jitconfigvalues.h

@@ -521,6 +521,12 @@ CONFIG_INTEGER(TC_OnStackReplacement, W("TC_OnStackReplacement"), 0)
 CONFIG_INTEGER(TC_OnStackReplacement_InitialCounter, W("TC_OnStackReplacement_InitialCounter"), 1000)
 // Enable partial compilation for Tier0 methods
 CONFIG_INTEGER(TC_PartialCompilation, W("TC_PartialCompilation"), 0)
+#if defined(DEBUG)
+// Randomly sprinkle patchpoints. Value is the likelyhood any given stack-empty point becomes a patchpoint.
+CONFIG_INTEGER(JitRandomOnStackReplacement, W("JitRandomOnStackReplacement"), 0)
+// Place patchpoint at the specified IL offset, if possible. Overrides random placement.
+CONFIG_INTEGER(JitOffsetOnStackReplacement, W("JitOffsetOnStackReplacement"), -1)
+#endif // debug
 
 #if defined(DEBUG)
 CONFIG_STRING(JitEnableOsrRange, W("JitEnableOsrRange")) // Enable osr for only some methods

src/coreclr/jit/lclvars.cpp

@@ -305,7 +305,9 @@ void Compiler::lvaInitTypeRef()
             JITDUMP("-- V%02u is OSR exposed\n", varNum);
             varDsc->lvHasLdAddrOp = 1;
 
-            if (varDsc->lvType != TYP_STRUCT) // Why does it apply only to non-structs?
+            // todo: Why does it apply only to non-structs?
+            //
+            if (!varTypeIsStruct(varDsc) && !varTypeIsSIMD(varDsc))
             {
                 lvaSetVarAddrExposed(varNum DEBUGARG(AddressExposedReason::OSR_EXPOSED));
             }

src/tests/Common/testenvironment.proj

@@ -62,6 +62,7 @@
       COMPlus_JitEdgeProfiling;
       COMPlus_JitRandomGuardedDevirtualization;
       COMPlus_JitRandomEdgeCounts;
+      COMPlus_JitRandomOnStackReplacement;
       RunningIlasmRoundTrip
     </COMPlusVariables>
   </PropertyGroup>
@@ -187,6 +188,7 @@
     <TestEnvironment Include="gcstress0xc_jitminopts_heapverify1" GCStress="0xC" JITMinOpts="1" HeapVerify="1" />
     <TestEnvironment Include="jitosr" TC_OnStackReplacement="1" TC_QuickJitForLoops="1" TieredCompilation="1" />
     <TestEnvironment Include="jitosr_stress" TC_OnStackReplacement="1" TC_QuickJitForLoops="1" TC_OnStackReplacement_InitialCounter="1" OSR_HitLimit="1" TieredCompilation="1" />
+    <TestEnvironment Include="jitosr_stress_random" TC_OnStackReplacement="1" TC_QuickJitForLoops="1" TC_OnStackReplacement_InitialCounter="1" OSR_HitLimit="2" TieredCompilation="1" JitRandomOnStackReplacement="15"/>
     <TestEnvironment Include="jitosr_pgo" TC_OnStackReplacement="1" TC_QuickJitForLoops="1" TieredCompilation="1" TieredPGO="1" />
     <TestEnvironment Include="jitpartialcompilation" TC_PartialCompilation="1" TC_QuickJitForLoops="1" TieredCompilation="1" />
     <TestEnvironment Include="jitpartialcompilation_pgo" TC_PartialCompilation="1" TC_QuickJitForLoops="1" TieredCompilation="1" TieredPGO="1" />