From 8c89c25ee007d0d36baa5053bf4de9645940b0f3 Mon Sep 17 00:00:00 2001 From: Carol Eidt Date: Wed, 13 Jun 2018 14:43:07 -0700 Subject: [PATCH] Fix enregistered lclFld bug (dotnet/coreclr#18418) * Fix enregistered lclFld bug In `impFixupStructReturnType()`, don't transform to `GT_LCL_FLD` if we have a scalar lclVar. Also, to avoid future bad codegen, add verification and recovery code to Lowering. Fix dotnet/coreclr#18408 * Extract the full conditions for whether a lclVar is a reg candidate, so it can be called from the assert in Lowering. * Review feedback Commit migrated from https://github.com/dotnet/coreclr/commit/82134a002fed96739694b6f085baaeea6c7c41f5 --- src/coreclr/src/jit/importer.cpp | 7 +- src/coreclr/src/jit/lower.cpp | 7 + src/coreclr/src/jit/lower.h | 17 ++ src/coreclr/src/jit/lowerarmarch.cpp | 5 + src/coreclr/src/jit/lowerxarch.cpp | 5 + src/coreclr/src/jit/lsra.cpp | 213 ++++++++++----------- .../JitBlue/GitHub_18408/GitHub_18408.cs | 122 ++++++++++++ .../JitBlue/GitHub_18408/GitHub_18408.csproj | 35 ++++ 8 files changed, 295 insertions(+), 116 deletions(-) create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18408/GitHub_18408.cs create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18408/GitHub_18408.csproj diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp index dc3a383..806a25c 100644 --- a/src/coreclr/src/jit/importer.cpp +++ b/src/coreclr/src/jit/importer.cpp @@ -8654,7 +8654,12 @@ REDO_RETURN_NODE: // and no normalizing if (op->gtOper == GT_LCL_VAR) { - op->ChangeOper(GT_LCL_FLD); + // It is possible that we now have a lclVar of scalar type. + // If so, don't transform it to GT_LCL_FLD. + if (varTypeIsStruct(lvaTable[op->AsLclVar()->gtLclNum].lvType)) + { + op->ChangeOper(GT_LCL_FLD); + } } else if (op->gtOper == GT_OBJ) { diff --git a/src/coreclr/src/jit/lower.cpp b/src/coreclr/src/jit/lower.cpp index f45dadc..defc34c 100644 --- a/src/coreclr/src/jit/lower.cpp +++ b/src/coreclr/src/jit/lower.cpp @@ -269,6 +269,13 @@ GenTree* Lowering::LowerNode(GenTree* node) break; #endif // FEATURE_HW_INTRINSICS + case GT_LCL_FLD: + { + // We should only encounter this for lclVars that are lvDoNotEnregister. + verifyLclFldDoNotEnregister(node->AsLclVarCommon()->gtLclNum); + break; + } + case GT_LCL_VAR: WidenSIMD12IfNecessary(node->AsLclVarCommon()); break; diff --git a/src/coreclr/src/jit/lower.h b/src/coreclr/src/jit/lower.h index e98dc08..d9ecb23 100644 --- a/src/coreclr/src/jit/lower.h +++ b/src/coreclr/src/jit/lower.h @@ -355,6 +355,23 @@ private: return LIR::AsRange(m_block); } + // Any tracked lclVar accessed by a LCL_FLD or STORE_LCL_FLD should be marked doNotEnregister. + // This method checks, and asserts in the DEBUG case if it is not so marked, + // but in the non-DEBUG case (asserts disabled) set the flag so that we don't generate bad code. + // This ensures that the local's value is valid on-stack as expected for a *LCL_FLD. + void verifyLclFldDoNotEnregister(unsigned lclNum) + { + LclVarDsc* varDsc = &(comp->lvaTable[lclNum]); + // Do a couple of simple checks before setting lvDoNotEnregister. + // This may not cover all cases in 'isRegCandidate()' but we don't want to + // do an expensive check here. For non-candidates it is not harmful to set lvDoNotEnregister. + if (varDsc->lvTracked && !varDsc->lvDoNotEnregister) + { + assert(!m_lsra->isRegCandidate(varDsc)); + comp->lvaSetVarDoNotEnregister(lclNum DEBUG_ARG(Compiler::DNER_LocalField)); + } + } + LinearScan* m_lsra; unsigned vtableCallTemp; // local variable we use as a temp for vtable calls SideEffectSet m_scratchSideEffects; // SideEffectSet used for IsSafeToContainMem and isRMWIndirCandidate diff --git a/src/coreclr/src/jit/lowerarmarch.cpp b/src/coreclr/src/jit/lowerarmarch.cpp index 47998fe..85d8caa 100644 --- a/src/coreclr/src/jit/lowerarmarch.cpp +++ b/src/coreclr/src/jit/lowerarmarch.cpp @@ -203,6 +203,11 @@ void Lowering::LowerStoreLoc(GenTreeLclVarCommon* storeLoc) } } } + if (storeLoc->OperIs(GT_STORE_LCL_FLD)) + { + // We should only encounter this for lclVars that are lvDoNotEnregister. + verifyLclFldDoNotEnregister(storeLoc->gtLclNum); + } ContainCheckStoreLoc(storeLoc); } diff --git a/src/coreclr/src/jit/lowerxarch.cpp b/src/coreclr/src/jit/lowerxarch.cpp index a0a4dfe..c912a2d 100644 --- a/src/coreclr/src/jit/lowerxarch.cpp +++ b/src/coreclr/src/jit/lowerxarch.cpp @@ -94,6 +94,11 @@ void Lowering::LowerStoreLoc(GenTreeLclVarCommon* storeLoc) } } } + if (storeLoc->OperIs(GT_STORE_LCL_FLD)) + { + // We should only encounter this for lclVars that are lvDoNotEnregister. + verifyLclFldDoNotEnregister(storeLoc->gtLclNum); + } ContainCheckStoreLoc(storeLoc); } diff --git a/src/coreclr/src/jit/lsra.cpp b/src/coreclr/src/jit/lsra.cpp index 5d5baea..a5e676f 100644 --- a/src/coreclr/src/jit/lsra.cpp +++ b/src/coreclr/src/jit/lsra.cpp @@ -1357,7 +1357,10 @@ void LinearScan::identifyCandidatesExceptionDataflow() bool LinearScan::isRegCandidate(LclVarDsc* varDsc) { - // We shouldn't be called if opt settings do not permit register variables. + if (!enregisterLocalVars) + { + return false; + } assert((compiler->opts.compFlags & CLFLG_REGVAR) != 0); if (!varDsc->lvTracked) @@ -1386,6 +1389,95 @@ bool LinearScan::isRegCandidate(LclVarDsc* varDsc) { return false; } + + // Don't enregister if the ref count is zero. + if (varDsc->lvRefCnt == 0) + { + varDsc->lvRefCntWtd = 0; + return false; + } + + // Variables that are address-exposed are never enregistered, or tracked. + // A struct may be promoted, and a struct that fits in a register may be fully enregistered. + // Pinned variables may not be tracked (a condition of the GCInfo representation) + // or enregistered, on x86 -- it is believed that we can enregister pinned (more properly, "pinning") + // references when using the general GC encoding. + unsigned lclNum = (unsigned)(varDsc - compiler->lvaTable); + if (varDsc->lvAddrExposed || !varTypeIsEnregisterableStruct(varDsc)) + { +#ifdef DEBUG + Compiler::DoNotEnregisterReason dner = Compiler::DNER_AddrExposed; + if (!varDsc->lvAddrExposed) + { + dner = Compiler::DNER_IsStruct; + } +#endif // DEBUG + compiler->lvaSetVarDoNotEnregister(lclNum DEBUGARG(dner)); + return false; + } + else if (varDsc->lvPinned) + { + varDsc->lvTracked = 0; +#ifdef JIT32_GCENCODER + compiler->lvaSetVarDoNotEnregister(lclNum DEBUGARG(Compiler::DNER_PinningRef)); +#endif // JIT32_GCENCODER + return false; + } + + // Are we not optimizing and we have exception handlers? + // if so mark all args and locals as volatile, so that they + // won't ever get enregistered. + // + if (compiler->opts.MinOpts() && compiler->compHndBBtabCount > 0) + { + compiler->lvaSetVarDoNotEnregister(lclNum DEBUGARG(Compiler::DNER_LiveInOutOfHandler)); + } + + if (varDsc->lvDoNotEnregister) + { + return false; + } + + switch (genActualType(varDsc->TypeGet())) + { +#if CPU_HAS_FP_SUPPORT + case TYP_FLOAT: + case TYP_DOUBLE: + return !compiler->opts.compDbgCode; + +#endif // CPU_HAS_FP_SUPPORT + + case TYP_INT: + case TYP_LONG: + case TYP_REF: + case TYP_BYREF: + break; + +#ifdef FEATURE_SIMD + case TYP_SIMD12: + case TYP_SIMD16: + case TYP_SIMD32: + return !varDsc->lvPromoted; + + // TODO-1stClassStructs: Move TYP_SIMD8 up with the other SIMD types, after handling the param issue + // (passing & returning as TYP_LONG). + case TYP_SIMD8: + return false; +#endif // FEATURE_SIMD + + case TYP_STRUCT: + return false; + + case TYP_UNDEF: + case TYP_UNKNOWN: + noway_assert(!"lvType not set correctly"); + varDsc->lvType = TYP_INT; + return false; + + default: + return false; + } + return true; } @@ -1544,19 +1636,7 @@ void LinearScan::identifyCandidates() } #endif // DOUBLE_ALIGN - /* Track all locals that can be enregistered */ - - if (!isRegCandidate(varDsc)) - { - varDsc->lvLRACandidate = 0; - if (varDsc->lvTracked) - { - localVarIntervals[varDsc->lvVarIndex] = nullptr; - } - continue; - } - - assert(varDsc->lvTracked); + // Start with the assumption that it's a candidate. varDsc->lvLRACandidate = 1; @@ -1564,116 +1644,19 @@ void LinearScan::identifyCandidates() // the same register assignment throughout varDsc->lvRegister = false; - /* If the ref count is zero */ - if (varDsc->lvRefCnt == 0) - { - /* Zero ref count, make this untracked */ - varDsc->lvRefCntWtd = 0; - varDsc->lvLRACandidate = 0; - } - - // Variables that are address-exposed are never enregistered, or tracked. - // A struct may be promoted, and a struct that fits in a register may be fully enregistered. - // Pinned variables may not be tracked (a condition of the GCInfo representation) - // or enregistered, on x86 -- it is believed that we can enregister pinned (more properly, "pinning") - // references when using the general GC encoding. - - if (varDsc->lvAddrExposed || !varTypeIsEnregisterableStruct(varDsc)) + if (!isRegCandidate(varDsc)) { varDsc->lvLRACandidate = 0; -#ifdef DEBUG - Compiler::DoNotEnregisterReason dner = Compiler::DNER_AddrExposed; - if (!varDsc->lvAddrExposed) + if (varDsc->lvTracked) { - dner = Compiler::DNER_IsStruct; + localVarIntervals[varDsc->lvVarIndex] = nullptr; } -#endif // DEBUG - compiler->lvaSetVarDoNotEnregister(lclNum DEBUGARG(dner)); - } - else if (varDsc->lvPinned) - { - varDsc->lvTracked = 0; -#ifdef JIT32_GCENCODER - compiler->lvaSetVarDoNotEnregister(lclNum DEBUGARG(Compiler::DNER_PinningRef)); -#endif // JIT32_GCENCODER - } - - // Are we not optimizing and we have exception handlers? - // if so mark all args and locals as volatile, so that they - // won't ever get enregistered. - // - if (compiler->opts.MinOpts() && compiler->compHndBBtabCount > 0) - { - compiler->lvaSetVarDoNotEnregister(lclNum DEBUGARG(Compiler::DNER_LiveInOutOfHandler)); - } - - if (varDsc->lvDoNotEnregister) - { - varDsc->lvLRACandidate = 0; - localVarIntervals[varDsc->lvVarIndex] = nullptr; continue; } - var_types type = genActualType(varDsc->TypeGet()); - - switch (type) - { -#if CPU_HAS_FP_SUPPORT - case TYP_FLOAT: - case TYP_DOUBLE: - if (compiler->opts.compDbgCode) - { - varDsc->lvLRACandidate = 0; - } -#ifdef ARM_SOFTFP - if (varDsc->lvIsParam && varDsc->lvIsRegArg) - { - type = (type == TYP_DOUBLE) ? TYP_LONG : TYP_INT; - } -#endif // ARM_SOFTFP - break; -#endif // CPU_HAS_FP_SUPPORT - - case TYP_INT: - case TYP_LONG: - case TYP_REF: - case TYP_BYREF: - break; - -#ifdef FEATURE_SIMD - case TYP_SIMD12: - case TYP_SIMD16: - case TYP_SIMD32: - if (varDsc->lvPromoted) - { - varDsc->lvLRACandidate = 0; - } - break; - - // TODO-1stClassStructs: Move TYP_SIMD8 up with the other SIMD types, after handling the param issue - // (passing & returning as TYP_LONG). - case TYP_SIMD8: -#endif // FEATURE_SIMD - - case TYP_STRUCT: - { - varDsc->lvLRACandidate = 0; - } - break; - - case TYP_UNDEF: - case TYP_UNKNOWN: - noway_assert(!"lvType not set correctly"); - varDsc->lvType = TYP_INT; - - __fallthrough; - - default: - varDsc->lvLRACandidate = 0; - } - if (varDsc->lvLRACandidate) { + var_types type = genActualType(varDsc->TypeGet()); Interval* newInt = newInterval(type); newInt->setLocalNumber(compiler, lclNum, this); VarSetOps::AddElemD(compiler, registerCandidateVars, varDsc->lvVarIndex); diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18408/GitHub_18408.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18408/GitHub_18408.cs new file mode 100644 index 0000000..6b0399d --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18408/GitHub_18408.cs @@ -0,0 +1,122 @@ +// 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. + +using System; +using System.Reflection; +using System.Threading; +using System.Runtime.CompilerServices; + +class MetadataReader +{ + [MethodImpl(MethodImplOptions.NoInlining)] + public Method GetMethod(MethodHandle handle) + { + return new Method(this, handle, MethodAttributes.Abstract); + } + +} + +struct Handle +{ + int _value; + + public MethodHandle ToMethodHandle(MetadataReader reader) + { + return new MethodHandle(this); + } + + public int GetValue() + { + return _value; + } +} + +static class MetadataReaderExtensions +{ + public static unsafe Handle AsHandle(this int token) + { + return *(Handle*)&token; + } +} + +struct MethodHandle +{ + internal int _value; + + [MethodImpl(MethodImplOptions.NoInlining)] + internal MethodHandle(Handle value) + { + _value = value.GetValue(); + } + + public Method GetMethod(MetadataReader reader) + { + return reader.GetMethod(this); + } +} + +struct Method +{ + internal MetadataReader _reader; + internal MethodHandle _handle; + internal MethodAttributes _flags; + + public Method(MetadataReader r, MethodHandle h, MethodAttributes f) + { + _reader = r; + _handle = h; + _flags = f; + } + + public MethodAttributes Flags => _flags; +} + +struct QMethodDefinition +{ + private QMethodDefinition(MetadataReader reader, int token) + { + _reader = reader; + _handle = token; + } + + public static QMethodDefinition FromObjectAndInt(MetadataReader reader, int token) + { + return new QMethodDefinition(reader, token); + } + + public MetadataReader Reader { get { return _reader; } } + public int Token { get { return _handle; } } + + public bool IsValid { get { return _reader == null; } } + + private readonly MetadataReader _reader; + private readonly int _handle; + + public MetadataReader NativeFormatReader { get { return _reader; } } + public MethodHandle NativeFormatHandle { get { return _handle.AsHandle().ToMethodHandle(NativeFormatReader); } } +} + +class GitHub_18408 +{ + [MethodImpl(MethodImplOptions.NoInlining)] + static object foo(QMethodDefinition methodHandle) + { + Method method = methodHandle.NativeFormatHandle.GetMethod(methodHandle.NativeFormatReader); + return (method.Flags != (MethodAttributes)0) ? new object() : null; + } + + public static int Main(string[] args) + { + MetadataReader r = new MetadataReader(); + + if (foo(QMethodDefinition.FromObjectAndInt(r, 1)) == null) + { + Console.WriteLine("FAIL"); + return -1; + } + + Console.WriteLine("PASS"); + return 100; + } +} diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18408/GitHub_18408.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18408/GitHub_18408.csproj new file mode 100644 index 0000000..0b9fa76 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18408/GitHub_18408.csproj @@ -0,0 +1,35 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + + + + + + False + + + + + True + True + + + + + + + + + + -- 2.7.4