Skip to content

Commit 66e5e7f

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
1 parent a96445d commit 66e5e7f

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
@@ -1003,6 +1003,14 @@ struct GenTree
10031003
#define GTF_ICON_FIELD_OFF 0x08000000 // GT_CNS_INT -- constant is a field offset
10041004
#define GTF_ICON_SIMD_COUNT 0x04000000 // GT_CNS_INT -- constant is Vector<T>.Count
10051005

1006+
#define GTF_ICON_INITCLASS 0x02000000 // GT_CNS_INT -- Constant is used to access a static that requires preceding
1007+
// class/static init helper. In some cases, the constant is
1008+
// the address of the static field itself, and in other cases
1009+
// there's an extra layer of indirection and it is the address
1010+
// of the cell that the runtime will fill in with the address
1011+
// of the static field; in both of those cases, the constant
1012+
// is what gets flagged.
1013+
10061014
#define GTF_BLK_VOLATILE GTF_IND_VOLATILE // GT_ASG, GT_STORE_BLK, GT_STORE_OBJ, GT_STORE_DYNBLK -- is a volatile block operation
10071015
#define GTF_BLK_UNALIGNED GTF_IND_UNALIGNED // GT_ASG, GT_STORE_BLK, GT_STORE_OBJ, GT_STORE_DYNBLK -- is an unaligned block operation
10081016

‎src/jit/importer.cpp‎

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

60986098
/* Create the data member node */
6099-
if (pFldAddr == nullptr)
6099+
op1 = gtNewIconHandleNode(pFldAddr == nullptr ? (size_t)fldAddr : (size_t)pFldAddr, GTF_ICON_STATIC_HDL,
6100+
fldSeq);
6101+
6102+
if (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_INITCLASS)
61006103
{
6101-
op1 = gtNewIconHandleNode((size_t)fldAddr, GTF_ICON_STATIC_HDL, fldSeq);
6104+
op1->gtFlags |= GTF_ICON_INITCLASS;
61026105
}
6103-
else
6104-
{
6105-
op1 = gtNewIconHandleNode((size_t)pFldAddr, GTF_ICON_STATIC_HDL, fldSeq);
61066106

6107+
if (pFldAddr != nullptr)
6108+
{
61076109
// There are two cases here, either the static is RVA based,
61086110
// in which case the type of the FIELD node is not a GC type
61096111
// 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
@@ -6573,6 +6573,13 @@ GenTreePtr Compiler::fgMorphField(GenTreePtr tree, MorphAddrContext* mac)
65736573

65746574
GenTreePtr tlsRef = gtNewIconHandleNode(WIN32_TLS_SLOTS, GTF_ICON_TLS_HDL);
65756575

6576+
// Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS
6577+
if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0)
6578+
{
6579+
tree->gtFlags &= ~GTF_FLD_INITCLASS;
6580+
tlsRef->gtFlags |= GTF_ICON_INITCLASS;
6581+
}
6582+
65766583
tlsRef = gtNewOperNode(GT_IND, TYP_I_IMPL, tlsRef);
65776584

65786585
if (dllRef != nullptr)
@@ -6627,6 +6634,12 @@ GenTreePtr Compiler::fgMorphField(GenTreePtr tree, MorphAddrContext* mac)
66276634
FieldSeqNode* fieldSeq =
66286635
fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd);
66296636
addr->gtIntCon.gtFieldSeq = fieldSeq;
6637+
// Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS
6638+
if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0)
6639+
{
6640+
tree->gtFlags &= ~GTF_FLD_INITCLASS;
6641+
addr->gtFlags |= GTF_ICON_INITCLASS;
6642+
}
66306643

66316644
tree->SetOper(GT_IND);
66326645
// The GTF_FLD_NULLCHECK is the same bit as GTF_IND_ARR_LEN.
@@ -6658,6 +6671,13 @@ GenTreePtr Compiler::fgMorphField(GenTreePtr tree, MorphAddrContext* mac)
66586671
{
66596672
GenTreePtr addr = gtNewIconHandleNode((size_t)pFldAddr, GTF_ICON_STATIC_HDL);
66606673

6674+
// Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS
6675+
if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0)
6676+
{
6677+
tree->gtFlags &= ~GTF_FLD_INITCLASS;
6678+
addr->gtFlags |= GTF_ICON_INITCLASS;
6679+
}
6680+
66616681
// There are two cases here, either the static is RVA based,
66626682
// in which case the type of the FIELD node is not a GC type
66636683
// 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)