From 973f1eb8f72eac6d3df25a5e6f7b60986f10d202 Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Wed, 28 Apr 2021 12:02:41 -0700 Subject: [PATCH] Liveness fix for struct enreg. (#51851) * Improve mustInit for structs. * fix linux * review response. --- src/coreclr/jit/codegencommon.cpp | 58 +++++++++++++++++++++++-------- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/lower.cpp | 22 ++++++++++-- src/coreclr/jit/optimizer.cpp | 3 +- 4 files changed, 66 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 4adf6b234da54..0abdff90cc640 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -4747,15 +4747,21 @@ void CodeGen::genCheckUseBlockInit() continue; } - if (varDsc->lvIsTemp && !varDsc->HasGCPtr()) + const bool isTemp = varDsc->lvIsTemp; + const bool hasGCPtr = varDsc->HasGCPtr(); + const bool isTracked = varDsc->lvTracked; + const bool isStruct = varTypeIsStruct(varDsc); + const bool compInitMem = compiler->info.compInitMem; + + if (isTemp && !hasGCPtr) { varDsc->lvMustInit = 0; continue; } - if (compiler->info.compInitMem || varDsc->HasGCPtr() || varDsc->lvMustInit) + if (compInitMem || hasGCPtr || varDsc->lvMustInit) { - if (varDsc->lvTracked) + if (isTracked) { /* For uninitialized use of tracked variables, the liveness * will bubble to the top (compiler->fgFirstBB) in fgInterBlockLocalVarLiveness() @@ -4794,20 +4800,42 @@ void CodeGen::genCheckUseBlockInit() } } - /* With compInitMem, all untracked vars will have to be init'ed */ - /* VSW 102460 - Do not force initialization of compiler generated temps, - unless they are untracked GC type or structs that contain GC pointers */ - CLANG_FORMAT_COMMENT_ANCHOR; - - if ((!varDsc->lvTracked || (varDsc->lvType == TYP_STRUCT)) && varDsc->lvOnFrame) + if (varDsc->lvOnFrame) { - - varDsc->lvMustInit = true; - - if (!counted) + bool mustInitThisVar = false; + if (hasGCPtr && !isTracked) + { + JITDUMP("must init V%02u because it has a GC ref\n", varNum); + mustInitThisVar = true; + } + else if (hasGCPtr && isStruct) + { + // TODO-1stClassStructs: support precise liveness reporting for such structs. + JITDUMP("must init a tracked V%02u because it a struct with a GC ref\n", varNum); + mustInitThisVar = true; + } + else { - initStkLclCnt += roundUp(compiler->lvaLclSize(varNum), TARGET_POINTER_SIZE) / sizeof(int); - counted = true; + // We are done with tracked or GC vars, now look at untracked vars without GC refs. + if (!isTracked) + { + assert(!hasGCPtr && !isTemp); + if (compInitMem) + { + JITDUMP("must init V%02u because compInitMem is set and it is not a temp\n", varNum); + mustInitThisVar = true; + } + } + } + if (mustInitThisVar) + { + varDsc->lvMustInit = true; + + if (!counted) + { + initStkLclCnt += roundUp(compiler->lvaLclSize(varNum), TARGET_POINTER_SIZE) / sizeof(int); + counted = true; + } } } } diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 80c0f856c2172..fe85376d8312a 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -984,7 +984,7 @@ class LclVarDsc var_types lvaArgType(); // Returns true if this variable contains GC pointers (including being a GC pointer itself). - bool HasGCPtr() + bool HasGCPtr() const { return varTypeIsGC(lvType) || ((lvType == TYP_STRUCT) && m_layout->HasGCPtr()); } diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 63d7a7f6f6daf..f72943b7f95e1 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -5915,17 +5915,35 @@ void Lowering::CheckNode(Compiler* compiler, GenTree* node) { unsigned lclNum = node->AsLclVarCommon()->GetLclNum(); LclVarDsc* lclVar = &compiler->lvaTable[lclNum]; -#ifdef DEBUG if (node->TypeIs(TYP_SIMD12)) { assert(compiler->lvaIsFieldOfDependentlyPromotedStruct(lclVar) || (lclVar->lvSize() == 12)); } -#endif // DEBUG } break; #endif // TARGET_64BIT #endif // SIMD + case GT_LCL_VAR_ADDR: + case GT_LCL_FLD_ADDR: + { + const GenTreeLclVarCommon* lclVarAddr = node->AsLclVarCommon(); + const LclVarDsc* varDsc = compiler->lvaGetDesc(lclVarAddr); + if (((lclVarAddr->gtFlags & GTF_VAR_DEF) != 0) && varDsc->HasGCPtr()) + { + // Emitter does not correctly handle live updates for LCL_VAR_ADDR + // when they are not contained, for example, `STOREIND byref(GT_LCL_VAR_ADDR not-contained)` + // would generate: + // add r1, sp, 48 // r1 contains address of a lclVar V01. + // str r0, [r1] // a gc ref becomes live in V01, but emitter would not report it. + // Make sure that we use uncontained address nodes only for variables + // that will be marked as mustInit and will be alive throughout the whole block even when tracked. + assert(lclVarAddr->isContained() || !varDsc->lvTracked || varTypeIsStruct(varDsc)); + // TODO: support this assert for uses, see https://github.com/dotnet/runtime/issues/51900. + } + break; + } + default: break; } diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 498bb249ff85b..e624834117190 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -9924,7 +9924,8 @@ void Compiler::optRemoveRedundantZeroInits() if (BitVecOps::IsMember(&bitVecTraits, zeroInitLocals, lclNum) || (lclDsc->lvIsStructField && BitVecOps::IsMember(&bitVecTraits, zeroInitLocals, lclDsc->lvParentLcl)) || - (!lclDsc->lvTracked && !fgVarNeedsExplicitZeroInit(lclNum, bbInALoop, bbIsReturn))) + ((!lclDsc->lvTracked || !isEntire) && + !fgVarNeedsExplicitZeroInit(lclNum, bbInALoop, bbIsReturn))) { // We are guaranteed to have a zero initialization in the prolog or a // dominating explicit zero initialization and the local hasn't been redefined