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

Commit 673f56a

Browse files
JosephTremouletJosephTremoulet
committed
Stop hoisting statics above cctors
Extend the logic in optLoopHoist around `CLS_VAR` nodes to ensure we don't hoist any tree that contains a `CLS_VAR` for a field with the flag `CORINFO_FLG_FIELD_INITCLASS` set, unless we also (identify and) hoist the corresponding static init helper call. The previous logic was insufficient in that it blocked hoisting of singleton `CLS_VAR` nodes, but not hoisting of trees that contain `CLS_VAR` nodes as sub-trees. Add flag `GTF_FLD_INITCLASS`/`GTF_CLS_VAR_INITCLASS` so that optLoopHoist can recall which fields are paired with static init helper calls, and only block the hoisting of their trees. Fixes #10780.
1 parent eac27ca commit 673f56a

File tree

7 files changed

+174
-26
lines changed

7 files changed

+174
-26
lines changed

‎src/jit/compiler.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5005,7 +5005,8 @@ class Compiler
50055005
unsigned lnum,
50065006
LoopHoistContext* hoistCtxt,
50075007
bool* firstBlockAndBeforeSideEffect,
5008-
bool* pHoistable);
5008+
bool* pHoistable,
5009+
bool* pCctorDependent);
50095010

50105011
// Performs the hoisting 'tree' into the PreHeader for loop 'lnum'
50115012
void optHoistCandidate(GenTreePtr tree, unsigned lnum, LoopHoistContext* hoistCtxt);

‎src/jit/gentree.h‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -926,6 +926,7 @@ struct GenTree
926926

927927
#define GTF_FLD_NULLCHECK 0x80000000 // GT_FIELD -- need to nullcheck the "this" pointer
928928
#define GTF_FLD_VOLATILE 0x40000000 // GT_FIELD/GT_CLS_VAR -- same as GTF_IND_VOLATILE
929+
#define GTF_FLD_INITCLASS 0x20000000 // GT_FIELD/GT_CLS_VAR -- field access requires preceding class/static init helper
929930

930931
#define GTF_INX_RNGCHK 0x80000000 // GT_INDEX -- the array reference should be range-checked.
931932
#define GTF_INX_REFARR_LAYOUT 0x20000000 // GT_INDEX -- same as GTF_IND_REFARR_LAYOUT
@@ -955,8 +956,10 @@ struct GenTree
955956
(GTF_IND_VOLATILE | GTF_IND_REFARR_LAYOUT | GTF_IND_TGTANYWHERE | GTF_IND_NONFAULTING | GTF_IND_TLS_REF | \
956957
GTF_IND_UNALIGNED | GTF_IND_INVARIANT | GTF_IND_ARR_INDEX)
957958

958-
#define GTF_CLS_VAR_ASG_LHS 0x04000000 // GT_CLS_VAR -- this GT_CLS_VAR node is (the effective val) of the LHS
959-
// of an assignment; don't evaluate it independently.
959+
#define GTF_CLS_VAR_ASG_LHS 0x04000000 // GT_CLS_VAR -- this GT_CLS_VAR node is (the effective val) of the LHS
960+
// of an assignment; don't evaluate it independently.
961+
#define GTF_CLS_VAR_VOLATILE 0x40000000 // GT_FIELD/GT_CLS_VAR -- same as GTF_IND_VOLATILE
962+
#define GTF_CLS_VAR_INITCLASS 0x20000000 // GT_FIELD/GT_CLS_VAR -- same as GTF_FLD_INITCLASS
960963

961964
#define GTF_ADDR_ONSTACK 0x80000000 // GT_ADDR -- this expression is guaranteed to be on the stack
962965

‎src/jit/importer.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6057,6 +6057,11 @@ GenTreePtr Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolve
60576057
// In future, it may be better to just create the right tree here instead of folding it later.
60586058
op1 = gtNewFieldRef(lclTyp, pResolvedToken->hField);
60596059

6060+
if (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_INITCLASS)
6061+
{
6062+
op1->gtFlags |= GTF_FLD_INITCLASS;
6063+
}
6064+
60606065
if (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_STATIC_IN_HEAP)
60616066
{
60626067
op1->gtType = TYP_REF; // points at boxed object

‎src/jit/morph.cpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6628,9 +6628,10 @@ GenTreePtr Compiler::fgMorphField(GenTreePtr tree, MorphAddrContext* mac)
66286628
else
66296629
#endif // _TARGET_64BIT_
66306630
{
6631-
// Only volatile could be set, and it maps over
6632-
noway_assert((tree->gtFlags & ~(GTF_FLD_VOLATILE | GTF_COMMON_MASK)) == 0);
6633-
noway_assert(GTF_FLD_VOLATILE == GTF_IND_VOLATILE);
6631+
// Only volatile or classinit could be set, and they map over
6632+
noway_assert((tree->gtFlags & ~(GTF_FLD_VOLATILE | GTF_FLD_INITCLASS | GTF_COMMON_MASK)) == 0);
6633+
static_assert_no_msg(GTF_FLD_VOLATILE == GTF_CLS_VAR_VOLATILE);
6634+
static_assert_no_msg(GTF_FLD_INITCLASS == GTF_CLS_VAR_INITCLASS);
66346635
tree->SetOper(GT_CLS_VAR);
66356636
tree->gtClsVar.gtClsVarHnd = symHnd;
66366637
FieldSeqNode* fieldSeq =

‎src/jit/optimizer.cpp‎

Lines changed: 53 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6008,7 +6008,9 @@ void Compiler::optHoistLoopExprsForBlock(BasicBlock* blk, unsigned lnum, LoopHoi
60086008
{
60096009
GenTreePtr stmtTree = stmt->gtStmtExpr;
60106010
bool hoistable;
6011-
(void)optHoistLoopExprsForTree(stmtTree, lnum, hoistCtxt, &firstBlockAndBeforeSideEffect, &hoistable);
6011+
bool cctorDependent;
6012+
(void)optHoistLoopExprsForTree(stmtTree, lnum, hoistCtxt, &firstBlockAndBeforeSideEffect, &hoistable,
6013+
&cctorDependent);
60126014
if (hoistable)
60136015
{
60146016
// we will try to hoist the top-level stmtTree
@@ -6114,43 +6116,81 @@ bool Compiler::optIsProfitableToHoistableTree(GenTreePtr tree, unsigned lnum)
61146116

61156117
//
61166118
// This function returns true if 'tree' is a loop invariant expression.
6117-
// It also sets '*pHoistable' to true if 'tree' can be hoisted into a loop PreHeader block
6119+
// It also sets '*pHoistable' to true if 'tree' can be hoisted into a loop PreHeader block,
6120+
// and sets '*pCctorDependent' if 'tree' is a function of a static field that must not be
6121+
// hoisted (even if '*pHoistable' is true) unless a preceding corresponding cctor init helper
6122+
// call is also hoisted.
61186123
//
6119-
bool Compiler::optHoistLoopExprsForTree(
6120-
GenTreePtr tree, unsigned lnum, LoopHoistContext* hoistCtxt, bool* pFirstBlockAndBeforeSideEffect, bool* pHoistable)
6124+
bool Compiler::optHoistLoopExprsForTree(GenTreePtr tree,
6125+
unsigned lnum,
6126+
LoopHoistContext* hoistCtxt,
6127+
bool* pFirstBlockAndBeforeSideEffect,
6128+
bool* pHoistable,
6129+
bool* pCctorDependent)
61216130
{
61226131
// First do the children.
61236132
// We must keep track of whether each child node was hoistable or not
61246133
//
61256134
unsigned nChildren = tree->NumChildren();
61266135
bool childrenHoistable[GenTree::MAX_CHILDREN];
6136+
bool childrenCctorDependent[GenTree::MAX_CHILDREN];
61276137

61286138
// Initialize the array elements for childrenHoistable[] to false
61296139
for (unsigned i = 0; i < nChildren; i++)
61306140
{
6131-
childrenHoistable[i] = false;
6141+
childrenHoistable[i] = false;
6142+
childrenCctorDependent[i] = false;
61326143
}
61336144

6134-
bool treeIsInvariant = true;
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;
61356148
for (unsigned childNum = 0; childNum < nChildren; childNum++)
61366149
{
61376150
if (!optHoistLoopExprsForTree(tree->GetChild(childNum), lnum, hoistCtxt, pFirstBlockAndBeforeSideEffect,
6138-
&childrenHoistable[childNum]))
6151+
&childrenHoistable[childNum], &childrenCctorDependent[childNum]))
61396152
{
61406153
treeIsInvariant = false;
61416154
}
6155+
6156+
if (childrenCctorDependent[childNum])
6157+
{
6158+
// Normally, a parent of a cctor-dependent tree is also cctor-dependent.
6159+
treeIsCctorDependent = true;
6160+
6161+
// Check for the case where we can stop propagating cctor-dependent upwards.
6162+
if (tree->OperIs(GT_COMMA) && (childNum == 1))
6163+
{
6164+
GenTreePtr op1 = tree->gtGetOp1();
6165+
if (op1->OperIs(GT_CALL))
6166+
{
6167+
GenTreeCall* call = op1->AsCall();
6168+
if ((call->gtCallType == CT_HELPER) &&
6169+
s_helperCallProperties.MayRunCctor(eeGetHelperNum(call->gtCallMethHnd)))
6170+
{
6171+
// Hoisting the comma is ok because it would hoist the initialization along
6172+
// with the static field reference.
6173+
treeIsCctorDependent = false;
6174+
// Hoisting the static field without hoisting the initialization would be
6175+
// incorrect; unset childrenHoistable for the field to ensure this doesn't
6176+
// happen.
6177+
childrenHoistable[0] = false;
6178+
}
6179+
}
6180+
}
6181+
}
61426182
}
61436183

6144-
// If all the children of "tree" are hoistable, then "tree" itself can be hoisted
6145-
//
6146-
bool treeIsHoistable = treeIsInvariant;
6184+
// If all the children of "tree" are hoistable, then "tree" itself can be hoisted,
6185+
// unless it has a static var reference that can't be hoisted past its cctor call.
6186+
bool treeIsHoistable = treeIsInvariant && !treeIsCctorDependent;
61476187

61486188
// But we must see if anything else prevents "tree" from being hoisted.
61496189
//
61506190
if (treeIsInvariant)
61516191
{
61526192
// Tree must be a suitable CSE candidate for us to be able to hoist it.
6153-
treeIsHoistable = optIsCSEcandidate(tree);
6193+
treeIsHoistable &= optIsCSEcandidate(tree);
61546194

61556195
// If it's a call, it must be a helper call, and be pure.
61566196
// Further, if it may run a cctor, it must be labeled as "Hoistable"
@@ -6189,14 +6229,6 @@ bool Compiler::optHoistLoopExprsForTree(
61896229
treeIsHoistable = false;
61906230
}
61916231
}
6192-
// Currently we must give up on reads from static variables (even if we are in the first block).
6193-
//
6194-
if (tree->OperGet() == GT_CLS_VAR)
6195-
{
6196-
// TODO-CQ: test that fails if we hoist GT_CLS_VAR: JIT\Directed\Languages\ComponentPascal\pi_r.exe
6197-
// method Main
6198-
treeIsHoistable = false;
6199-
}
62006232
}
62016233

62026234
// Is the value of the whole tree loop invariant?
@@ -6290,7 +6322,8 @@ bool Compiler::optHoistLoopExprsForTree(
62906322
}
62916323
}
62926324

6293-
*pHoistable = treeIsHoistable;
6325+
*pHoistable = treeIsHoistable;
6326+
*pCctorDependent = treeIsCctorDependent;
62946327
return treeIsInvariant;
62956328
}
62966329

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
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+
public struct Pair
13+
{
14+
public int Left;
15+
public int Right;
16+
17+
public static Pair TenFour = new Pair() { Left = 10, Right = 4 };
18+
}
19+
20+
static class C
21+
{
22+
static int Sum;
23+
static int Two;
24+
25+
// Bug repro requires a use of a Pair value; this is a small fn that takes
26+
// a Pair by value to serve as that use. Inline it aggressively so that
27+
// we won't think the call might kill the static field.
28+
[MethodImpl(MethodImplOptions.AggressiveInlining)]
29+
static void Accumulate(Pair pair)
30+
{
31+
Sum += pair.Left + pair.Right;
32+
}
33+
34+
35+
[MethodImpl(MethodImplOptions.NoInlining)]
36+
static void SumNFourteens(int n)
37+
{
38+
for (int i = 0; i < n; ++i)
39+
{
40+
Two = 2; // Store to C.Two here is a global side-effect above which we won't hoist the static initializer (since it may throw).
41+
Accumulate(Pair.TenFour); // Hoisting the load of Pair.TenFour above the static init call is incorrect.
42+
}
43+
}
44+
45+
public static int Main(string[] args)
46+
{
47+
Sum = 0;
48+
SumNFourteens(7); // Now Sum = 14 * 7 = 98 (and Two = 2)
49+
return Sum + Two; // 98 + 2 = 100 on success
50+
}
51+
}
52+
}

‎tests/src/JIT/Regression/JitBlue/GitHub_10780/GitHub_10780.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>{1D93D1C3-458A-44ED-ABCC-7D0739B28C92}</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_10780.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)