diff --git a/src/jit/gentree.h b/src/jit/gentree.h index 68fbeb7faac9..c1edcb47f75b 100644 --- a/src/jit/gentree.h +++ b/src/jit/gentree.h @@ -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.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 diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 65bd12e1334c..048fbbc904d2 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -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 diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index 662717562edf..7d759e26e3fb 100644 --- a/src/jit/morph.cpp +++ b/src/jit/morph.cpp @@ -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 + 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) @@ -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. @@ -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 diff --git a/src/jit/optimizer.cpp b/src/jit/optimizer.cpp index d951422e574b..1e50e537e012 100644 --- a/src/jit/optimizer.cpp +++ b/src/jit/optimizer.cpp @@ -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. + 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, @@ -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]); } } } diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_11689/GitHub_11689.cs b/tests/src/JIT/Regression/JitBlue/GitHub_11689/GitHub_11689.cs new file mode 100644 index 000000000000..3d8428c928cc --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_11689/GitHub_11689.cs @@ -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); + } + } +} diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_11689/GitHub_11689.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_11689/GitHub_11689.csproj new file mode 100644 index 000000000000..52f50228df93 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_11689/GitHub_11689.csproj @@ -0,0 +1,53 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {A04B4F1F-62D3-4799-94AB-ABFB220415BE} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages + ..\..\ + + 7a9bfb7d + + + + + + + + + False + + + + + True + True + + + + + + + + + + + + + + +