Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: refactor to allow OSR to switch to optimized #61851

Merged
merged 5 commits into from
Nov 22, 2021

Conversation

AndyAyersMS
Copy link
Member

When OSR is enabled, the jit may still need to switch to optimized codegen if
there is something in the method that OSR cannot handle. Examples include:

  • localloc
  • tail. prefixes
  • loops in handlers
  • reverse pinvoke (currently bypasses Tiering so somewhat moot)

When OSR is enabled, rework the "switch to optimize logic" in the jit to check
for these cases up front before we start importing code.

When both QuickJitForLoops and OnStackReplacement are enabled, this should ensure
that if we can't transition out of long-running Tier0 code (via OSR) then we will
instead optimize the method. Very few methods overall should opt-out of Tier0.

Note some of these unhandled constructs can eventually be handled by OSR, with
additional work.

When OSR is enabled, the jit may still need to switch to optimized codegen if
there is something in the method that OSR cannot handle. Examples include:
* localloc
* tail. prefixes
* loops in handlers
* reverse pinvoke (currently bypasses Tiering so somewhat moot)

When OSR is enabled, rework the "switch to optimize logic" in the jit to check
for these cases up front before we start importing code.

When both QuickJitForLoops and OnStackReplacement are enabled, this should ensure
that if we can't transition out of long-running Tier0 code (via OSR) then we will
instead optimize the method. Very few methods overall should opt-out of Tier0.

Note some of these unhandled constructs can eventually be handled by OSR, with
additional work.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 19, 2021
@ghost
Copy link

ghost commented Nov 19, 2021

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

When OSR is enabled, the jit may still need to switch to optimized codegen if
there is something in the method that OSR cannot handle. Examples include:

  • localloc
  • tail. prefixes
  • loops in handlers
  • reverse pinvoke (currently bypasses Tiering so somewhat moot)

When OSR is enabled, rework the "switch to optimize logic" in the jit to check
for these cases up front before we start importing code.

When both QuickJitForLoops and OnStackReplacement are enabled, this should ensure
that if we can't transition out of long-running Tier0 code (via OSR) then we will
instead optimize the method. Very few methods overall should opt-out of Tier0.

Note some of these unhandled constructs can eventually be handled by OSR, with
additional work.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Nov 19, 2021

cc @dotnet/jit-contrib

This is the last "prereq" before we can consider turning OSR on by default for x64.

Note partial compilation doesn't have quite the same level of restriction -- it is opportunistic. If we can't support patchpoints in a method we can just bail out for each partial compilation candidate block, rather than forcing the method to be optimized.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

//
const char* reason = nullptr;

if ((info.compFlags & CORINFO_FLG_DISABLE_TIER0_FOR_LOOPS) == 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the wrong way around?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, yes. I should verify this is a no-diff change.

Also I'm changing what happens with BBINSTR -- formerly it would prevent switching to optimized -- as a result we'll have slightly fewer methods with PGO. Maybe I should rethink this.

We may need to distinguish the case where we we trap everything in Tier0 to gather static data from dynamic PGO. Right now we can't tell which is which. If we're trying to gather for static PGO we don't want to switch; if we're trying to minimize downside of Tier0 we do...

Copy link
Member Author

@AndyAyersMS AndyAyersMS Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should verify this is a no-diff change.

On further thought, it may not be no-diff because of the BBINSTR change, but there should be minimal diffs, and only in the ASP.NET cases where we see instrumentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all in the context of #58632, so perhaps I can refine this down to not switching to optimized for BBINSTR'd methods with explicit tail calls.

OSR doesn't support explicit tail calls yet but presumably if we don't optimize recursive tail calls to loops then eventually normal tiering swaps all this over.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This should replicate the current logic where we alwatys optimize methods
with explicit tail calls unless we're instrumenting them.

To make this work with OSR we simply won't put patchpoints in methods
with explicit tail calls, instead trusting that Tiering can get us
to optimized versions.
@AndyAyersMS
Copy link
Member Author

The interplay of QJFL, OSR, BBINSTR, and explicit tail calls is a bit messier than I'd like -- hopefully the latest commit here gets us to a reasonable point.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

One unexpected failure in osr_stress to investigate

;; JIT\opt\OSR\mainlooptry3

Assert failure(PID 5524 [0x00001594], Thread: 5004 [0x138c]): Assertion failed 'needLabel' in 'MainLoopCloselyNestedTry:Main():int' during 'Generate code' (IL size 121)

File: D:\a\_work\1\s\src\coreclr\jit\codegenlinear.cpp Line: 348

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Nov 20, 2021

This is from the new handling for alignment. We scan blocks for labels:

Mark labels for codegen
  BB01 : first block
  BB03 : branch target
  BB04 : branch target
  BB03 : branch target
...

and notably decide BB02 does not need a label. Later on we put an align after BB01:

Mapped BB01 to G_M6245_IG02
Label: IG02, GCvars=0000000000000000 {}, gcrefRegs=00000000 {}, byrefRegs=00000000 {}

Scope info: begin block BB01, IL range [???..???)
Scope info: ignoring block beginning
Added IP mapping: NO_MAP (G_M6245_IG02,ins#0,ofs#0) label
Scope info: ignoring block end
IN0001:        jmp      L_M6245_BB03
IN0002:        align    [15 bytes]
Adding 'align' instruction of 15 bytes in G_M6245_IG02.

Variable Live Range History Dump for BB01
V00 loc0: rbp[124] (1 slot) [(G_M6245_IG02,ins#0,ofs#0), ...]
V01 loc1: rcx [(G_M6245_IG02,ins#0,ofs#0), ...]
V02 loc2: rdx [(G_M6245_IG02,ins#0,ofs#0), ...]

=============== Generating BB02 [???..???), preds={} succs={BB03} flags=0x00000000.20000070: keep i internal LIR 
BB02 IN (3)={V01 V02 V00} + ByrefExposed + GcHeap
     OUT(3)={V01 V02 V00} + ByrefExposed + GcHeap

and then assert because BB02 does not have a label.

In this case BB02 is an unreachable kept block:

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight   IBC  lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0020]  1  1                          1           [???..???)-> BB03 (always) T1      try {       keep i internal try label LIR 
BB02 [0021]  0  1                          1           [???..???)                 T1                  keep i internal LIR 
BB03 [0002]  3  0    BB01,BB02,BB05        8           [012..016)                 T0      try {       keep i try Loop label bwd bwd-target LIR 
BB04 [0003]  2  0    BB03,BB04            32         0 [016..02E)-> BB04 ( cond ) T0                  i Loop label bwd bwd-target LIR align 
BB05 [0005]  1  0    BB04                  8           [02E..03A)-> BB03 ( cond ) T0                  i bwd LIR 
BB06 [0007]  1  0    BB05                  1           [03A..03E)-> BB08 (always) T0      }           i LIR 
BB07 [0017]  1  1    BB13                  0           [???..???)                 T1      }           i internal rare label LIR 
BB08 [0018]  2       BB06,BB07             1           [???..???)-> BB14 (callf )                     i internal label LIR 
BB09 [0019]  1       BB28                  1           [???..???)-> BB10 (ALWAYS)                     i internal gcsafe LIR KEEP 
BB10 [0010]  1       BB09                  1           [06C..074)-> BB12 ( cond )                     i label gcsafe LIR 
BB11 [0011]  1       BB10                  1           [074..076)        (return)                     i gcsafe LIR 
BB12 [0012]  1       BB10                  1           [076..079)        (return)                     i label gcsafe LIR 
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ funclets follow
BB13 [0008]  1  1  0                       0           [03E..041)-> BB07 ( cret ) T1 H0 F catch { }   keep i rare label flet LIR 
BB14 [0009]  2     1 BB08                  1    100    [041..06C)                    H1 F finally {   keep i label hascall flet IBC LIR 
BB15 [0028]  1     1 BB14                  1    100    [041..042)-> BB17 ( cond )    H1               keep i hascall gcsafe IBC LIR 
BB16 [0031]  1     1 BB15                  0      0    [041..042)-> BB18 (always)    H1               i rare gcsafe IBC LIR 
BB17 [0032]  1     1 BB15                  1    100    [041..042)                    H1               i label gcsafe idxlen nullcheck IBC LIR 
BB18 [0036]  2     1 BB16,BB17             1    100    [04B..04C)-> BB20 ( cond )    H1               keep i label gcsafe IBC LIR 
BB19 [0053]  1     1 BB18                  0      0    [04B..04C)        (throw )    H1               i rare hascall gcsafe IBC LIR 
BB20 [0054]  1     1 BB18                  0.77  77    [04B..04C)-> BB22 ( cond )    H1               i label gcsafe IBC LIR 
BB21 [0060]  1     1 BB20                  0.77        [04B..04C)                    H1               i hascall gcsafe LIR 
BB22 [0061]  2     1 BB20,BB21             0.77  77    [04B..04C)-> BB27 ( cond )    H1               i label gcsafe IBC LIR 
BB23 [0064]  1     1 BB22                  0.76  76    [04B..04C)-> BB28 (always)    H1               i hascall gcsafe IBC LIR 
BB27 [0050]  1     1 BB22                  0.01   1    [04B..04C)                    H1               i label hascall gcsafe IBC LIR 
BB28 [0047]  2     1 BB23,BB27             1    100    [???..???)        (finret)    H1   }           keep internal label hascall gcsafe IBC LIR 
-----------------------------------------------------------------------------------------------------------------------------------------

There is no reason to have BB02 kept -- it comes from the OSR step block expansion. Fix is easy enough.

@AndyAyersMS
Copy link
Member Author

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

Experimental CI failures are down to the "expected ones".

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants