Skip to content

Commit f1f9fde

Browse files
AndyAyersMSAndyAyersMS
authored
JIT: import entire method for OSR, prune unneeded parts later (#83910)
For OSR compiles, always import from the original entry point in addition to the OSR entry point. This gives the OSR compiler a chance to see all of the method and so properly compute address exposure, instead of relying on the Tier0 analysis. Once address exposure has been determined, revoke special protection for the original entry and try and prune away blocks that are no longer needed (we actually wait until morph). Fixes #83783. May also fix some of the cases where OSR perf is lagging (though don't expect this to fix them all).
1 parent 8ae68db commit f1f9fde

11 files changed

+152
-60
lines changed

‎src/coreclr/jit/compiler.h

+3-1
Original file line numberDiff line numberDiff line change
@@ -4420,7 +4420,9 @@ class Compiler
44204420
DomTreeNode* fgSsaDomTree;
44214421

44224422
bool fgBBVarSetsInited;
4423-
bool fgOSROriginalEntryBBProtected;
4423+
4424+
// Track how many artificial ref counts we've added to fgEntryBB (for OSR)
4425+
unsigned fgEntryBBExtraRefs;
44244426

44254427
// Allocate array like T* a = new T[fgBBNumMax + 1];
44264428
// Using helper so we don't keep forgetting +1.

‎src/coreclr/jit/fgbasic.cpp

+6-6
Original file line numberDiff line numberDiff line change
@@ -43,12 +43,12 @@ void Compiler::fgInit()
4343

4444
/* Initialize the basic block list */
4545

46-
fgFirstBB = nullptr;
47-
fgLastBB = nullptr;
48-
fgFirstColdBlock = nullptr;
49-
fgEntryBB = nullptr;
50-
fgOSREntryBB = nullptr;
51-
fgOSROriginalEntryBBProtected = false;
46+
fgFirstBB = nullptr;
47+
fgLastBB = nullptr;
48+
fgFirstColdBlock = nullptr;
49+
fgEntryBB = nullptr;
50+
fgOSREntryBB = nullptr;
51+
fgEntryBBExtraRefs = 0;
5252

5353
#if defined(FEATURE_EH_FUNCLETS)
5454
fgFirstFuncletBB = nullptr;

‎src/coreclr/jit/fgdiagnostic.cpp

+5-4
Original file line numberDiff line numberDiff line change
@@ -2888,11 +2888,12 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef
28882888
blockRefs += 1;
28892889
}
28902890

2891-
// Under OSR, if we also are keeping the original method entry around,
2892-
// mark that as implicitly referenced as well.
2893-
if (opts.IsOSR() && (block == fgEntryBB) && fgOSROriginalEntryBBProtected)
2891+
// Under OSR, if we also are keeping the original method entry around
2892+
// via artifical ref counts, account for those.
2893+
//
2894+
if (opts.IsOSR() && (block == fgEntryBB))
28942895
{
2895-
blockRefs += 1;
2896+
blockRefs += fgEntryBBExtraRefs;
28962897
}
28972898

28982899
/* Check the bbRefs */

‎src/coreclr/jit/importer.cpp

+11
Original file line numberDiff line numberDiff line change
@@ -12624,6 +12624,17 @@ void Compiler::impImport()
1262412624
// which we should verify over when we find jump targets.
1262512625
impImportBlockPending(entryBlock);
1262612626

12627+
if (opts.IsOSR())
12628+
{
12629+
// We now import all the IR and keep it around so we can
12630+
// analyze address exposure more robustly.
12631+
//
12632+
JITDUMP("OSR: protecting original method entry " FMT_BB "\n", fgEntryBB->bbNum);
12633+
impImportBlockPending(fgEntryBB);
12634+
fgEntryBB->bbRefs++;
12635+
fgEntryBBExtraRefs++;
12636+
}
12637+
1262712638
/* Import blocks in the worker-list until there are no more */
1262812639

1262912640
while (impPendingList)

‎src/coreclr/jit/importercalls.cpp

-28
Original file line numberDiff line numberDiff line change
@@ -1295,34 +1295,6 @@ var_types Compiler::impImportCall(OPCODE opcode,
12951295
successor->bbFlags |= BBF_TAILCALL_SUCCESSOR;
12961296
optMethodFlags |= OMF_HAS_TAILCALL_SUCCESSOR;
12971297
}
1298-
1299-
// If this call might eventually turn into a loop back to method entry, make sure we
1300-
// import the method entry.
1301-
//
1302-
assert(call->IsCall());
1303-
GenTreeCall* const actualCall = call->AsCall();
1304-
const bool mustImportEntryBlock = gtIsRecursiveCall(methHnd) || actualCall->IsInlineCandidate() ||
1305-
actualCall->IsGuardedDevirtualizationCandidate();
1306-
1307-
// Only schedule importation if we're not currently importing the entry BB.
1308-
//
1309-
if (opts.IsOSR() && mustImportEntryBlock && (compCurBB != fgEntryBB))
1310-
{
1311-
JITDUMP("\nOSR: inlineable or recursive tail call [%06u] in the method, so scheduling " FMT_BB
1312-
" for importation\n",
1313-
dspTreeID(call), fgEntryBB->bbNum);
1314-
impImportBlockPending(fgEntryBB);
1315-
1316-
if (!fgOSROriginalEntryBBProtected && (fgEntryBB != fgFirstBB))
1317-
{
1318-
// Protect fgEntryBB from deletion, since it may not have any
1319-
// explicit flow references until morph.
1320-
//
1321-
fgEntryBB->bbRefs += 1;
1322-
fgOSROriginalEntryBBProtected = true;
1323-
JITDUMP(" also protecting original method entry " FMT_BB "\n", fgEntryBB->bbNum);
1324-
}
1325-
}
13261298
}
13271299
}
13281300

‎src/coreclr/jit/lclvars.cpp

+1-14
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,7 @@ void Compiler::lvaInitTypeRef()
323323
}
324324
}
325325

326-
// If this is an OSR method, mark all the OSR locals and model OSR exposure.
326+
// If this is an OSR method, mark all the OSR locals.
327327
//
328328
// Do this before we add the GS Cookie Dummy or Outgoing args to the locals
329329
// so we don't have to do special checks to exclude them.
@@ -334,19 +334,6 @@ void Compiler::lvaInitTypeRef()
334334
{
335335
LclVarDsc* const varDsc = lvaGetDesc(lclNum);
336336
varDsc->lvIsOSRLocal = true;
337-
338-
if (info.compPatchpointInfo->IsExposed(lclNum))
339-
{
340-
JITDUMP("-- V%02u is OSR exposed\n", lclNum);
341-
varDsc->lvHasLdAddrOp = 1;
342-
343-
// todo: Why does it apply only to non-structs?
344-
//
345-
if (!varTypeIsStruct(varDsc) && !varTypeIsSIMD(varDsc))
346-
{
347-
lvaSetVarAddrExposed(lclNum DEBUGARG(AddressExposedReason::OSR_EXPOSED));
348-
}
349-
}
350337
}
351338
}
352339

‎src/coreclr/jit/morph.cpp

+6-7
Original file line numberDiff line numberDiff line change
@@ -13865,14 +13865,13 @@ void Compiler::fgMorphBlocks()
1386513865
//
1386613866
if (opts.IsOSR() && (fgEntryBB != nullptr))
1386713867
{
13868-
if (fgOSROriginalEntryBBProtected)
13869-
{
13870-
JITDUMP("OSR: un-protecting original method entry " FMT_BB "\n", fgEntryBB->bbNum);
13871-
assert(fgEntryBB->bbRefs > 0);
13872-
fgEntryBB->bbRefs--;
13873-
}
13868+
JITDUMP("OSR: un-protecting original method entry " FMT_BB "\n", fgEntryBB->bbNum);
13869+
assert(fgEntryBBExtraRefs == 1);
13870+
assert(fgEntryBB->bbRefs >= 1);
13871+
fgEntryBB->bbRefs--;
13872+
fgEntryBBExtraRefs = 0;
1387413873

13875-
// And we don't need to remember this block anymore.
13874+
// We don't need to remember this block anymore.
1387613875
fgEntryBB = nullptr;
1387713876
}
1387813877

‎src/tests/JIT/opt/OSR/exposure1.cs

+47
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using System.Runtime.CompilerServices;
6+
7+
// Runtime 83738: need to ensure that 's' in 'Foo'
8+
// is marked as address exposed during OSR compiles.
9+
10+
public class Exposure1
11+
{
12+
[MethodImpl(MethodImplOptions.NoInlining)]
13+
static void Bar()
14+
{
15+
}
16+
17+
[MethodImpl(MethodImplOptions.NoInlining)]
18+
static bool Foo(int n)
19+
{
20+
S s = new S { F = 1234 };
21+
ref int foo = ref s.F;
22+
23+
for (int i = 0; i < n; i++)
24+
{
25+
Bar();
26+
}
27+
28+
int abc = s.F * 3 + 4;
29+
foo = 25;
30+
int def = s.F * 3 + 4;
31+
32+
int eabc = 1234 * 3 + 4;
33+
int edef = 25 * 3 + 4;
34+
Console.WriteLine("abc = {0} (expected {1}), def = {2} (expected {3})", abc, eabc, def, edef);
35+
return (abc == eabc && def == edef);
36+
}
37+
38+
public static int Main()
39+
{
40+
return Foo(50000) ? 100 : -1;
41+
}
42+
43+
public struct S
44+
{
45+
public int F;
46+
}
47+
}

‎src/tests/JIT/opt/OSR/exposure1.csproj

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
<PropertyGroup>
3+
<OutputType>Exe</OutputType>
4+
<DebugType />
5+
<Optimize>True</Optimize>
6+
</PropertyGroup>
7+
<ItemGroup>
8+
<Compile Include="$(MSBuildProjectName).cs" />
9+
<CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="1" />
10+
<CLRTestEnvironmentVariable Include="DOTNET_TC_QuickJitForLoops" Value="1" />
11+
<CLRTestEnvironmentVariable Include="DOTNET_TC_OnStackReplacement" Value="1" />
12+
</ItemGroup>
13+
</Project>

‎src/tests/JIT/opt/OSR/exposure2.cs

+47
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using System.Runtime.CompilerServices;
6+
7+
// Runtime 83738: need to ensure that 's' in 'Foo'
8+
// is marked as address exposed during OSR compiles.
9+
10+
public class Exposure2
11+
{
12+
[MethodImpl(MethodImplOptions.NoInlining)]
13+
static void Bar()
14+
{
15+
}
16+
17+
[MethodImpl(MethodImplOptions.NoInlining)]
18+
static bool Foo(int n)
19+
{
20+
S s = new S { F = 1234 };
21+
ref S foo = ref s;
22+
23+
for (int i = 0; i < n; i++)
24+
{
25+
Bar();
26+
}
27+
28+
int abc = s.F * 3 + 4;
29+
foo.F = 25;
30+
int def = s.F * 3 + 4;
31+
32+
int eabc = 1234 * 3 + 4;
33+
int edef = 25 * 3 + 4;
34+
Console.WriteLine("abc = {0} (expected {1}), def = {2} (expected {3})", abc, eabc, def, edef);
35+
return (abc == eabc && def == edef);
36+
}
37+
38+
public static int Main()
39+
{
40+
return Foo(50000) ? 100 : -1;
41+
}
42+
43+
public struct S
44+
{
45+
public int F;
46+
}
47+
}

‎src/tests/JIT/opt/OSR/exposure2.csproj

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
<PropertyGroup>
3+
<OutputType>Exe</OutputType>
4+
<DebugType />
5+
<Optimize>True</Optimize>
6+
</PropertyGroup>
7+
<ItemGroup>
8+
<Compile Include="$(MSBuildProjectName).cs" />
9+
<CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="1" />
10+
<CLRTestEnvironmentVariable Include="DOTNET_TC_QuickJitForLoops" Value="1" />
11+
<CLRTestEnvironmentVariable Include="DOTNET_TC_OnStackReplacement" Value="1" />
12+
</ItemGroup>
13+
</Project>

0 commit comments

Comments
 (0)