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

Don't hoist IConHandle statics above cctors #11694

Merged
merged 2 commits into from
May 23, 2017
Merged

Don't hoist IConHandle statics above cctors #11694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66e5e7f
Don't hoist IConHandle statics above cctors
JosephTremoulet May 17, 2017
357318f
Fix off-by-one error
JosephTremoulet May 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1003,6 +1003,14 @@ struct GenTree
#define GTF_ICON_FIELD_OFF 0x08000000 // GT_CNS_INT -- constant is a field offset
#define GTF_ICON_SIMD_COUNT 0x04000000 // GT_CNS_INT -- constant is Vector<T>.Count

#define GTF_ICON_INITCLASS 0x02000000 // GT_CNS_INT -- Constant is used to access a static that requires preceding
// class/static init helper. In some cases, the constant is
// the address of the static field itself, and in other cases
// there's an extra layer of indirection and it is the address
// of the cell that the runtime will fill in with the address
// of the static field; in both of those cases, the constant
// is what gets flagged.

#define GTF_BLK_VOLATILE GTF_IND_VOLATILE // GT_ASG, GT_STORE_BLK, GT_STORE_OBJ, GT_STORE_DYNBLK -- is a volatile block operation
#define GTF_BLK_UNALIGNED GTF_IND_UNALIGNED // GT_ASG, GT_STORE_BLK, GT_STORE_OBJ, GT_STORE_DYNBLK -- is an unaligned block operation

Expand Down
12 changes: 7 additions & 5 deletions src/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6096,14 +6096,16 @@ GenTreePtr Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolve
FieldSeqNode* fldSeq = GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField);

/* Create the data member node */
if (pFldAddr == nullptr)
op1 = gtNewIconHandleNode(pFldAddr == nullptr ? (size_t)fldAddr : (size_t)pFldAddr, GTF_ICON_STATIC_HDL,
fldSeq);

if (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_INITCLASS)
{
op1 = gtNewIconHandleNode((size_t)fldAddr, GTF_ICON_STATIC_HDL, fldSeq);
op1->gtFlags |= GTF_ICON_INITCLASS;
}
else
{
op1 = gtNewIconHandleNode((size_t)pFldAddr, GTF_ICON_STATIC_HDL, fldSeq);

if (pFldAddr != nullptr)
{
// There are two cases here, either the static is RVA based,
// in which case the type of the FIELD node is not a GC type
// and the handle to the RVA is a TYP_I_IMPL. Or the FIELD node is
Expand Down
20 changes: 20 additions & 0 deletions src/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6573,6 +6573,13 @@ GenTreePtr Compiler::fgMorphField(GenTreePtr tree, MorphAddrContext* mac)

GenTreePtr tlsRef = gtNewIconHandleNode(WIN32_TLS_SLOTS, GTF_ICON_TLS_HDL);

// Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS
Copy link

Choose a reason for hiding this comment

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

Also line 6468 contains a dangerous SetOper call which can cause the
flag GTF_FLD_INITCLASS to be interpreted as GTF_IND_REFARR_LAYOUT

Copy link
Author

Choose a reason for hiding this comment

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

6468 is dealing with instance fields (it's under the if (objRef) on line 6239 with else on line 6500)

Copy link

Choose a reason for hiding this comment

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

OK

if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0)
{
tree->gtFlags &= ~GTF_FLD_INITCLASS;
tlsRef->gtFlags |= GTF_ICON_INITCLASS;
}

tlsRef = gtNewOperNode(GT_IND, TYP_I_IMPL, tlsRef);

if (dllRef != nullptr)
Expand Down Expand Up @@ -6627,6 +6634,12 @@ GenTreePtr Compiler::fgMorphField(GenTreePtr tree, MorphAddrContext* mac)
FieldSeqNode* fieldSeq =
fieldMayOverlap ? FieldSeqStore::NotAField() : GetFieldSeqStore()->CreateSingleton(symHnd);
addr->gtIntCon.gtFieldSeq = fieldSeq;
// Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS
if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0)
{
tree->gtFlags &= ~GTF_FLD_INITCLASS;
addr->gtFlags |= GTF_ICON_INITCLASS;
}

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

// Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS
if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0)
{
tree->gtFlags &= ~GTF_FLD_INITCLASS;
addr->gtFlags |= GTF_ICON_INITCLASS;
}

// There are two cases here, either the static is RVA based,
// in which case the type of the FIELD node is not a GC type
// and the handle to the RVA is a TYP_I_IMPL. Or the FIELD node is
Expand Down
18 changes: 12 additions & 6 deletions src/jit/optimizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6142,9 +6142,15 @@ bool Compiler::optHoistLoopExprsForTree(GenTreePtr tree,
childrenCctorDependent[i] = false;
}

// Initclass CLS_VARs are the base case of cctor dependent trees.
bool treeIsCctorDependent = (tree->OperIs(GT_CLS_VAR) && ((tree->gtFlags & GTF_CLS_VAR_INITCLASS) != 0));
bool treeIsInvariant = true;
// Initclass CLS_VARs and IconHandles are the base cases of cctor dependent trees.
// In the IconHandle case, it's of course the dereference, rather than the constant itself, that is
// truly dependent on the cctor. So a more precise approach would be to separately propagate
// isCctorDependent and isAddressWhoseDereferenceWouldBeCctorDependent, but we don't for simplicity/throughput;
// the constant itself would be considered non-hoistable anyway, since optIsCSEcandidate returns
// false for constants.
Copy link

Choose a reason for hiding this comment

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

since optIsCSEcandidate returns false for constants.

This might not continue to be true in the future.
Is it possible to flag the indirection instead of the constant?
Or are we out of flag bits on GT_IND?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, GT_IND is already dipping into the common bits:

#define GTF_IND_NONFAULTING   0x00000800 // An indir that cannot fault.  GTF_SET_FLAGS is not used on indirs

We could (maybe?) similarly "steal" GTF_ZSF_SET or GTF_UNSIGNED for a new GT_IND flag, though I think the extra conservativism may be warranted here... with this change as-is, we'll block hoisting static field addresses produced by ldsflda (if the field is marked CORINFO_FLG_FIELD_INITCLASS and we aren't hoisting the cctor call with it). The importer does the marking now, but the importer doesn't have the context to know when a GT_IND might be fed by a prior ldsflda, so it seems risky to leave the ldsflda addresses unmarked. Using two bools here to track cctor-dependence would let us keep the annotations on the address and so avoid the flag sharing and the ldsflda+indir problem, I just didn't think it was worth it with no/little current benefit. I could be persuaded to change it though, what do you think?

Copy link

Choose a reason for hiding this comment

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

I'm comfortable with this fix for now and don't want to complicate this right now by fishing for extra flag bits for the GT_IND node.

bool treeIsCctorDependent = ((tree->OperIs(GT_CLS_VAR) && ((tree->gtFlags & GTF_CLS_VAR_INITCLASS) != 0)) ||
(tree->OperIs(GT_CNS_INT) && ((tree->gtFlags & GTF_ICON_INITCLASS) != 0)));
bool treeIsInvariant = true;
for (unsigned childNum = 0; childNum < nChildren; childNum++)
{
if (!optHoistLoopExprsForTree(tree->GetChild(childNum), lnum, hoistCtxt, pFirstBlockAndBeforeSideEffect,
Expand Down Expand Up @@ -6172,9 +6178,9 @@ bool Compiler::optHoistLoopExprsForTree(GenTreePtr tree,
// with the static field reference.
treeIsCctorDependent = false;
// Hoisting the static field without hoisting the initialization would be
// incorrect; unset childrenHoistable for the field to ensure this doesn't
// happen.
childrenHoistable[0] = false;
// incorrect, make sure we consider the field (which we flagged as
// cctor-dependent) non-hoistable.
noway_assert(!childrenHoistable[childNum]);
}
}
}
Expand Down
39 changes: 39 additions & 0 deletions tests/src/JIT/Regression/JitBlue/GitHub_11689/GitHub_11689.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

// Repro case for a bug involving hoisting of static field loads out of
// loops and (illegally) above the corresponding type initializer calls.

using System.Runtime.CompilerServices;

namespace N
{
struct WrappedInt
{
public int Value;

public static WrappedInt Twenty = new WrappedInt() { Value = 20 };
}
public static class C
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
static int unwrap(WrappedInt wi) => wi.Value;

[MethodImpl(MethodImplOptions.NoInlining)]
static int foo(int s, int n)
{
for (int i = 0; i < n; ++i)
{
s += unwrap(WrappedInt.Twenty); // Loading WrappedInt.Twenty must happen after calling the cctor
}

return s;
}

public static int Main(string[] args)
{
return foo(20, 4);
}
}
}
53 changes: 53 additions & 0 deletions tests/src/JIT/Regression/JitBlue/GitHub_11689/GitHub_11689.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
<PropertyGroup>
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
<AssemblyName>$(MSBuildProjectName)</AssemblyName>
<SchemaVersion>2.0</SchemaVersion>
<ProjectGuid>{A04B4F1F-62D3-4799-94AB-ABFB220415BE}</ProjectGuid>
<OutputType>Exe</OutputType>
<AppDesignerFolder>Properties</AppDesignerFolder>
<FileAlignment>512</FileAlignment>
<ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
<ReferencePath>$(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages</ReferencePath>
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>

<NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp>
</PropertyGroup>
<!-- Default configurations to help VS understand the configurations -->
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
</PropertyGroup>
<ItemGroup>
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
<Visible>False</Visible>
</CodeAnalysisDependentAssemblyPaths>
</ItemGroup>
<PropertyGroup>
<DebugType></DebugType>
<Optimize>True</Optimize>
<AllowUnsafeBlocks>True</AllowUnsafeBlocks>
</PropertyGroup>
<ItemGroup>
<Compile Include="GitHub_11689.cs" />
</ItemGroup>
<PropertyGroup>
<CLRTestBatchPreCommands><![CDATA[
$(CLRTestBatchPreCommands)
set COMPlus_TailcallStress=1
]]></CLRTestBatchPreCommands>
<BashCLRTestPreCommands><![CDATA[
$(BashCLRTestPreCommands)
export COMPlus_TailcallStress=1
]]></BashCLRTestPreCommands>
</PropertyGroup>
<ItemGroup>
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
</ItemGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
</PropertyGroup>
</Project>