Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit 1e9a45e

Browse files
JosephTremouletJosephTremoulet
committed
Don't hoist IConHandle statics above cctors
These constants appear loop invariant, hence so do their dereferences when a loop has no memory side-effects. Add flag `GTF_ICON_INITCLASS` similar to `GTF_FLD_INITCLASS`/`GTF_CLS_VAR_INITCLASS`, and use it to prevent hoisting such references without also hoisting their static init helper calls. Resolves #11689 [cherry-pick of 66e5e7f, with trivial conflict resolved]
1 parent 1b2a008 commit 1e9a45e

File tree

6 files changed

+136
-8
lines changed

6 files changed

+136
-8
lines changed

‎src/jit/gentree.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1007,6 +1007,14 @@ struct GenTree
10071007

10081008
#define GTF_ICON_SIMD_COUNT 0x04000000 // GT_CNS_INT -- constant is Vector<T>.Count
10091009

1010+
#define GTF_ICON_INITCLASS 0x02000000 // GT_CNS_INT -- Constant is used to access a static that requires preceding
1011+
// class/static init helper. In some cases, the constant is
1012+
// the address of the static field itself, and in other cases
1013+
// there's an extra layer of indirection and it is the address
1014+
// of the cell that the runtime will fill in with the address
1015+
// of the static field; in both of those cases, the constant
1016+
// is what gets flagged.
1017+
10101018
#define GTF_BLK_VOLATILE 0x40000000 // GT_ASG, GT_STORE_BLK, GT_STORE_OBJ, GT_STORE_DYNBLK
10111019
// -- is a volatile block operation
10121020
#define GTF_BLK_UNALIGNED 0x02000000 // GT_ASG, GT_STORE_BLK, GT_STORE_OBJ, GT_STORE_DYNBLK

‎src/jit/importer.cpp‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6093,14 +6093,16 @@ GenTreePtr Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolve
60936093
FieldSeqNode* fldSeq = GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField);
60946094

60956095
/* Create the data member node */
6096-
if (pFldAddr == nullptr)
6096+
op1 = gtNewIconHandleNode(pFldAddr == nullptr ? (size_t)fldAddr : (size_t)pFldAddr, GTF_ICON_STATIC_HDL,
6097+
fldSeq);
6098+
6099+
if (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_INITCLASS)
60976100
{
6098-
op1 = gtNewIconHandleNode((size_t)fldAddr, GTF_ICON_STATIC_HDL, fldSeq);
6101+
op1->gtFlags |= GTF_ICON_INITCLASS;
60996102
}
6100-
else
6101-
{
6102-
op1 = gtNewIconHandleNode((size_t)pFldAddr, GTF_ICON_STATIC_HDL, fldSeq);
61036103

6104+
if (pFldAddr != nullptr)
6105+
{
61046106
// There are two cases here, either the static is RVA based,
61056107
// in which case the type of the FIELD node is not a GC type
61066108
// and the handle to the RVA is a TYP_I_IMPL. Or the FIELD node is

‎src/jit/morph.cpp‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6560,6 +6560,13 @@ GenTreePtr Compiler::fgMorphField(GenTreePtr tree, MorphAddrContext* mac)
65606560

65616561
GenTreePtr tlsRef = gtNewIconHandleNode(WIN32_TLS_SLOTS, GTF_ICON_TLS_HDL);
65626562

6563+
// Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS
6564+
if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0)
6565+
{
6566+
tree->gtFlags &= ~GTF_FLD_INITCLASS;
6567+
tlsRef->gtFlags |= GTF_ICON_INITCLASS;
6568+
}
6569+
65636570
tlsRef = gtNewOperNode(GT_IND, TYP_I_IMPL, tlsRef);
65646571

65656572
if (dllRef != nullptr)
@@ -6614,6 +6621,12 @@ GenTreePtr Compiler::fgMorphField(GenTreePtr tree, MorphAddrContext* mac)
66146621
FieldSeqNode* fieldSeq =
66156622
fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd);
66166623
addr->gtIntCon.gtFieldSeq = fieldSeq;
6624+
// Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS
6625+
if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0)
6626+
{
6627+
tree->gtFlags &= ~GTF_FLD_INITCLASS;
6628+
addr->gtFlags |= GTF_ICON_INITCLASS;
6629+
}
66176630

66186631
tree->SetOper(GT_IND);
66196632
// The GTF_FLD_NULLCHECK is the same bit as GTF_IND_ARR_LEN.
@@ -6645,6 +6658,13 @@ GenTreePtr Compiler::fgMorphField(GenTreePtr tree, MorphAddrContext* mac)
66456658
{
66466659
GenTreePtr addr = gtNewIconHandleNode((size_t)pFldAddr, GTF_ICON_STATIC_HDL);
66476660

6661+
// Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS
6662+
if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0)
6663+
{
6664+
tree->gtFlags &= ~GTF_FLD_INITCLASS;
6665+
addr->gtFlags |= GTF_ICON_INITCLASS;
6666+
}
6667+
66486668
// There are two cases here, either the static is RVA based,
66496669
// in which case the type of the FIELD node is not a GC type
66506670
// and the handle to the RVA is a TYP_I_IMPL. Or the FIELD node is

‎src/jit/optimizer.cpp‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6142,9 +6142,15 @@ bool Compiler::optHoistLoopExprsForTree(GenTreePtr tree,
61426142
childrenCctorDependent[i] = false;
61436143
}
61446144

6145-
// Initclass CLS_VARs are the base case of cctor dependent trees.
6146-
bool treeIsCctorDependent = (tree->OperIs(GT_CLS_VAR) && ((tree->gtFlags & GTF_CLS_VAR_INITCLASS) != 0));
6147-
bool treeIsInvariant = true;
6145+
// Initclass CLS_VARs and IconHandles are the base cases of cctor dependent trees.
6146+
// In the IconHandle case, it's of course the dereference, rather than the constant itself, that is
6147+
// truly dependent on the cctor. So a more precise approach would be to separately propagate
6148+
// isCctorDependent and isAddressWhoseDereferenceWouldBeCctorDependent, but we don't for simplicity/throughput;
6149+
// the constant itself would be considered non-hoistable anyway, since optIsCSEcandidate returns
6150+
// false for constants.
6151+
bool treeIsCctorDependent = ((tree->OperIs(GT_CLS_VAR) && ((tree->gtFlags & GTF_CLS_VAR_INITCLASS) != 0)) ||
6152+
(tree->OperIs(GT_CNS_INT) && ((tree->gtFlags & GTF_ICON_INITCLASS) != 0)));
6153+
bool treeIsInvariant = true;
61486154
for (unsigned childNum = 0; childNum < nChildren; childNum++)
61496155
{
61506156
if (!optHoistLoopExprsForTree(tree->GetChild(childNum), lnum, hoistCtxt, pFirstBlockAndBeforeSideEffect,

‎tests/src/JIT/Regression/JitBlue/GitHub_11689/GitHub_11689.cs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
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+
// See the LICENSE file in the project root for more information.
4+
5+
// Repro case for a bug involving hoisting of static field loads out of
6+
// loops and (illegally) above the corresponding type initializer calls.
7+
8+
using System.Runtime.CompilerServices;
9+
10+
namespace N
11+
{
12+
struct WrappedInt
13+
{
14+
public int Value;
15+
16+
public static WrappedInt Twenty = new WrappedInt() { Value = 20 };
17+
}
18+
public static class C
19+
{
20+
[MethodImpl(MethodImplOptions.AggressiveInlining)]
21+
static int unwrap(WrappedInt wi) => wi.Value;
22+
23+
[MethodImpl(MethodImplOptions.NoInlining)]
24+
static int foo(int s, int n)
25+
{
26+
for (int i = 0; i < n; ++i)
27+
{
28+
s += unwrap(WrappedInt.Twenty); // Loading WrappedInt.Twenty must happen after calling the cctor
29+
}
30+
31+
return s;
32+
}
33+
34+
public static int Main(string[] args)
35+
{
36+
return foo(20, 4);
37+
}
38+
}
39+
}

‎tests/src/JIT/Regression/JitBlue/GitHub_11689/GitHub_11689.csproj‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
3+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
4+
<PropertyGroup>
5+
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
6+
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
7+
<AssemblyName>$(MSBuildProjectName)</AssemblyName>
8+
<SchemaVersion>2.0</SchemaVersion>
9+
<ProjectGuid>{A04B4F1F-62D3-4799-94AB-ABFB220415BE}</ProjectGuid>
10+
<OutputType>Exe</OutputType>
11+
<AppDesignerFolder>Properties</AppDesignerFolder>
12+
<FileAlignment>512</FileAlignment>
13+
<ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
14+
<ReferencePath>$(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages</ReferencePath>
15+
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
16+
17+
<NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp>
18+
</PropertyGroup>
19+
<!-- Default configurations to help VS understand the configurations -->
20+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
21+
</PropertyGroup>
22+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
23+
</PropertyGroup>
24+
<ItemGroup>
25+
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
26+
<Visible>False</Visible>
27+
</CodeAnalysisDependentAssemblyPaths>
28+
</ItemGroup>
29+
<PropertyGroup>
30+
<DebugType></DebugType>
31+
<Optimize>True</Optimize>
32+
<AllowUnsafeBlocks>True</AllowUnsafeBlocks>
33+
</PropertyGroup>
34+
<ItemGroup>
35+
<Compile Include="GitHub_11689.cs" />
36+
</ItemGroup>
37+
<PropertyGroup>
38+
<CLRTestBatchPreCommands><![CDATA[
39+
$(CLRTestBatchPreCommands)
40+
set COMPlus_TailcallStress=1
41+
]]></CLRTestBatchPreCommands>
42+
<BashCLRTestPreCommands><![CDATA[
43+
$(BashCLRTestPreCommands)
44+
export COMPlus_TailcallStress=1
45+
]]></BashCLRTestPreCommands>
46+
</PropertyGroup>
47+
<ItemGroup>
48+
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
49+
</ItemGroup>
50+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
51+
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
52+
</PropertyGroup>
53+
</Project>

0 commit comments

Comments
 (0)