JIT: refactor to allow OSR to switch to optimized

src/coreclr/jit/compiler.cpp

@@ -1873,6 +1873,7 @@ void Compiler::compInit(ArenaAllocator* pAlloc,
  compJmpOpUsed = false;
  compLongUsed = false;
  compTailCallUsed = false;
+ compTailPrefixSeen = false;
  compLocallocSeen = false;
  compLocallocUsed = false;
  compLocallocOptimized = false;
@@ -6272,7 +6273,8 @@ int Compiler::compCompileHelper(CORINFO_MODULE_HANDLE classPtr,
  info.compTotalColdCodeSize = 0;
  info.compClassProbeCount = 0;
 
- compHasBackwardJump = false;
+ compHasBackwardJump = false;
+ compHasBackwardJumpInHandler = false;
 
 #ifdef DEBUG
  compCurBB = nullptr;
@@ -6426,10 +6428,45 @@ int Compiler::compCompileHelper(CORINFO_MODULE_HANDLE classPtr,
  goto _Next;
  }
 
- if (compHasBackwardJump && (info.compFlags & CORINFO_FLG_DISABLE_TIER0_FOR_LOOPS) != 0 && fgCanSwitchToOptimized())
+ // We may decide to optimize this method,
+ // to avoid spending a long time stuck in Tier0 code.
+ //
+ if (fgCanSwitchToOptimized())
  {
- // Method likely has a loop, switch to the OptimizedTier to avoid spending too much time running slower code
- fgSwitchToOptimized();
+ // We only expect to be able to do this at Tier0.
+ //
+ assert(opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0));
+
+ // Honor the config setting that tells the jit to
+ // always optimize methods with loops or explicit tail calls.
+ //
+ // If that's not set, and OSR is enabled, the jit may still
+ // decide to optimize, if there's something in the method that
+ // OSR currently cannot handle.
+ //
+ const char* reason = nullptr;
+
+ if ((info.compFlags & CORINFO_FLG_DISABLE_TIER0_FOR_LOOPS) == 0)
+ {
+ if (compHasBackwardJump)
+ {
+ reason = "loop";
+ }
+ else if (compTailPrefixSeen)
+ {
+ reason = "tail.call";
+ }
+ }
+ else if (JitConfig.TC_OnStackReplacement() > 0)
+ {
+ const bool patchpointsOK = compCanHavePatchpoints(&reason);
+ assert(patchpointsOK || (reason != nullptr));
+ }
+
+ if (reason != nullptr)
+ {
+ fgSwitchToOptimized(reason);
+ }
  }
 
  compSetOptimizationLevel();

src/coreclr/jit/compiler.h

@@ -2985,6 +2985,8 @@ class Compiler
  bool fgNormalizeEHCase2();
  bool fgNormalizeEHCase3();
 
+ void fgCheckForLoopsInHandlers();
+
 #ifdef DEBUG
  void dispIncomingEHClause(unsigned num, const CORINFO_EH_CLAUSE& clause);
  void dispOutgoingEHClause(unsigned num, const CORINFO_EH_CLAUSE& clause);
@@ -6066,7 +6068,7 @@ class Compiler
  BasicBlock* fgLookupBB(unsigned addr);
 
  bool fgCanSwitchToOptimized();
- void fgSwitchToOptimized();
+ void fgSwitchToOptimized(const char* reason);
 
  bool fgMayExplicitTailCall();
 
@@ -9383,21 +9385,23 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 
  InlineResult* compInlineResult; // The result of importing the inlinee method.
 
- bool compDoAggressiveInlining; // If true, mark every method as CORINFO_FLG_FORCEINLINE
- bool compJmpOpUsed; // Does the method do a JMP
- bool compLongUsed; // Does the method use TYP_LONG
- bool compFloatingPointUsed; // Does the method use TYP_FLOAT or TYP_DOUBLE
- bool compTailCallUsed; // Does the method do a tailcall
- bool compLocallocSeen; // Does the method IL have localloc opcode
- bool compLocallocUsed; // Does the method use localloc.
- bool compLocallocOptimized; // Does the method have an optimized localloc
- bool compQmarkUsed; // Does the method use GT_QMARK/GT_COLON
- bool compQmarkRationalized; // Is it allowed to use a GT_QMARK/GT_COLON node.
- bool compUnsafeCastUsed; // Does the method use LDIND/STIND to cast between scalar/refernce types
- bool compHasBackwardJump; // Does the method (or some inlinee) have a lexically backwards jump?
- bool compSwitchedToOptimized; // Codegen initially was Tier0 but jit switched to FullOpts
- bool compSwitchedToMinOpts; // Codegen initially was Tier1/FullOpts but jit switched to MinOpts
- bool compSuppressedZeroInit; // There are vars with lvSuppressedZeroInit set
+ bool compDoAggressiveInlining; // If true, mark every method as CORINFO_FLG_FORCEINLINE
+ bool compJmpOpUsed; // Does the method do a JMP
+ bool compLongUsed; // Does the method use TYP_LONG
+ bool compFloatingPointUsed; // Does the method use TYP_FLOAT or TYP_DOUBLE
+ bool compTailCallUsed; // Does the method do a tailcall
+ bool compTailPrefixSeen; // Does the method IL have tail. prefix
+ bool compLocallocSeen; // Does the method IL have localloc opcode
+ bool compLocallocUsed; // Does the method use localloc.
+ bool compLocallocOptimized; // Does the method have an optimized localloc
+ bool compQmarkUsed; // Does the method use GT_QMARK/GT_COLON
+ bool compQmarkRationalized; // Is it allowed to use a GT_QMARK/GT_COLON node.
+ bool compUnsafeCastUsed; // Does the method use LDIND/STIND to cast between scalar/refernce types
+ bool compHasBackwardJump; // Does the method (or some inlinee) have a lexically backwards jump?
+ bool compHasBackwardJumpInHandler; // Does the method have a lexically backwards jump in a handler?
+ bool compSwitchedToOptimized; // Codegen initially was Tier0 but jit switched to FullOpts
+ bool compSwitchedToMinOpts; // Codegen initially was Tier1/FullOpts but jit switched to MinOpts
+ bool compSuppressedZeroInit; // There are vars with lvSuppressedZeroInit set
 
 // NOTE: These values are only reliable after
 // the importing is completely finished.

src/coreclr/jit/compiler.hpp

@@ -4736,6 +4736,14 @@ inline bool Compiler::compCanHavePatchpoints(const char** reason)
  {
  whyNot = "localloc";
  }
+ else if (compHasBackwardJumpInHandler)
+ {
+ whyNot = "loop in handler";
+ }
+ else if (compTailPrefixSeen)
+ {
+ whyNot = "tail.call";
+ }
  else if (opts.IsReversePInvoke())
  {
  whyNot = "reverse pinvoke";

src/coreclr/jit/fgbasic.cpp

@@ -2738,15 +2738,9 @@ unsigned Compiler::fgMakeBasicBlocks(const BYTE* codeAddr, IL_OFFSET codeSize, F
  BADCODE3("tail call not followed by ret", " at offset %04X", (IL_OFFSET)(codeAddr - codeBegp));
  }
 
- if (fgCanSwitchToOptimized() && fgMayExplicitTailCall())
+ if (fgMayExplicitTailCall())
  {
- // Method has an explicit tail call that may run like a loop or may not be generated as a tail
- // call in tier 0, switch to optimized to avoid spending too much time running slower code
- if (!opts.jitFlags->IsSet(JitFlags::JIT_FLAG_BBINSTR) ||
- ((info.compFlags & CORINFO_FLG_DISABLE_TIER0_FOR_LOOPS) != 0))
- {
- fgSwitchToOptimized();
- }
+ compTailPrefixSeen = true;
  }
  }
  else
@@ -3534,6 +3528,57 @@ void Compiler::fgFindBasicBlocks()
 #endif
 
  fgNormalizeEH();
+
+ fgCheckForLoopsInHandlers();
+}
+
+//------------------------------------------------------------------------
+// fgCheckForLoopsInHandlers: scan blocks seeing if any handler block
+// is a backedge target.
+//
+// Notes:
+// Sets compHasBackwardJumpInHandler if so. This will disable
+// setting patchpoints in this method and prompt the jit to
+// optimize the method instead.
+//
+// We assume any late-added handler (say for synchronized methods) will
+// not introduce any loops.
+//
+void Compiler::fgCheckForLoopsInHandlers()
+{
+ // We only care about this if we are going to set OSR patchpoints
+ // and the method has exception handling.
+ //
+ if (!opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0))
+ {
+ return;
+ }
+
+ if (JitConfig.TC_OnStackReplacement() == 0)
+ {
+ return;
+ }
+
+ if (info.compXcptnsCount == 0)
+ {
+ return;
+ }
+
+ // Walk blocks in handlers and filters, looing for a backedge target.
+ //
+ assert(!compHasBackwardJumpInHandler);
+ for (BasicBlock* const blk : Blocks())
+ {
+ if (blk->hasHndIndex())
+ {
+ if (blk->bbFlags & BBF_BACKWARD_JUMP_TARGET)
+ {
+ JITDUMP("\nHander block " FMT_BB "is backward jump target; can't have patchpoints in this method\n");
+ compHasBackwardJumpInHandler = true;
+ break;
+ }
+ }
+ }
 }
 
 //------------------------------------------------------------------------

src/coreclr/jit/flowgraph.cpp

@@ -524,6 +524,9 @@ bool Compiler::fgCanSwitchToOptimized()
 //------------------------------------------------------------------------
 // fgSwitchToOptimized: Switch the opt level from tier 0 to optimized
 //
+// Arguments:
+// reason - reason why opt level was switched
+//
 // Assumptions:
 // - fgCanSwitchToOptimized() is true
 // - compSetOptimizationLevel() has not been called
@@ -532,15 +535,16 @@ bool Compiler::fgCanSwitchToOptimized()
 // This method is to be called at some point before compSetOptimizationLevel() to switch the opt level to optimized
 // based on information gathered in early phases.
 
-void Compiler::fgSwitchToOptimized()
+void Compiler::fgSwitchToOptimized(const char* reason)
 {
  assert(fgCanSwitchToOptimized());
 
  // Switch to optimized and re-init options
- JITDUMP("****\n**** JIT Tier0 jit request switching to Tier1 because of loop\n****\n");
+ JITDUMP("****\n**** JIT Tier0 jit request switching to Tier1 because: %s\n****\n", reason);
  assert(opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0));
  opts.jitFlags->Clear(JitFlags::JIT_FLAG_TIER0);
  opts.jitFlags->Clear(JitFlags::JIT_FLAG_BBINSTR);
+ opts.jitFlags->Clear(JitFlags::JIT_FLAG_OSR);
 
  // Leave a note for jit diagnostics
  compSwitchedToOptimized = true;

src/coreclr/jit/importer.cpp

@@ -11591,24 +11591,27 @@ void Compiler::impImportBlockCode(BasicBlock* block)
 #ifdef FEATURE_ON_STACK_REPLACEMENT
 
  // Are there any places in the method where we might add a patchpoint?
+ //
  if (compHasBackwardJump)
  {
- // Are patchpoints enabled and supported?
- if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) && (JitConfig.TC_OnStackReplacement() > 0) &&
-  compCanHavePatchpoints())
+ // Is OSR enabled?
+ //
+ if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) && (JitConfig.TC_OnStackReplacement() > 0))
  {
+ 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?
- // Current strategy is blocks that are stack-empty and backwards branch targets and not in a handler
- //
- // Todo (perhaps): bail out of OSR and jit this method with optimization.
  //
- if (!block->hasHndIndex() && ((block->bbFlags & BBF_BACKWARD_JUMP_TARGET) != 0) &&
- (verCurrentState.esStackDepth == 0))
+ if (((block->bbFlags & BBF_BACKWARD_JUMP_TARGET) != 0) && (verCurrentState.esStackDepth == 0))
  {
+ // We should have noted this earlier and bailed out of OSR.
+ //
+ assert(!block->hasHndIndex());
+
  block->bbFlags |= BBF_PATCHPOINT;
  setMethodHasPatchpoint();
  }
@@ -11630,6 +11633,8 @@ void Compiler::impImportBlockCode(BasicBlock* block)
  // propagate rareness back through flow and place the partial compilation patchpoints "earlier"
  // so there are fewer overall.
  //
+ // Note unlike OSR, it's ok to forgo these.
+ //
  // Todo: stress mode...
  //
  if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) && (JitConfig.TC_PartialCompilation() > 0) &&

src/tests/JIT/opt/OSR/handlerloop.cs

@@ -0,0 +1,31 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+
+// OSR can't bail us out of a loop in a handler
+//
+class OSRHandlerLoop
+{
+ public static int Main()
+ {
+ int result = 0;
+ int expected = 704982705;
+ try
+ {
+ result++;
+ expected = 100000;
+ }
+ finally
+ {
+ for (int i = 0; i < 100_000; i++)
+ {
+ result += i;
+ }
+ }
+
+ Console.WriteLine($"{result} expected {expected}");
+
+ return (result == expected) ? 100 : -1;
+ }
+}

src/tests/JIT/opt/OSR/handlerloop.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>