From 2671dec89da88367c31c2859bf58fc525977e46c Mon Sep 17 00:00:00 2001 From: Eugene Rozenfeld Date: Thu, 11 Jun 2020 18:30:38 -0700 Subject: [PATCH] Fixes and improvements for removal of redundant zero inits 1. Bug fix: when processing a zero assignment to a field of a promoted struct, check the reference count of the parent struct. 2. Bug fix: when processing a zero assignment to a promoted struct, check the reference counts of all fields. 3. Bug fix and improvement: a dependently promoted field is initialized in the prolog iff its parent struct is initialized in the prolog so we can remove a field zero initialization if the parent struct is already zero initialized. 4. Improvement: keep track of explicitly zero-initialized locals so that duplicate explicit zero initializations can be eliminated. Fixes #37666. --- src/coreclr/src/jit/compiler.hpp | 7 + src/coreclr/src/jit/optimizer.cpp | 121 ++++++++++++------ .../JitBlue/GitHub_37666/GitHub_37666.cs | 71 ++++++++++ .../JitBlue/GitHub_37666/GitHub_37666.csproj | 13 ++ 4 files changed, 172 insertions(+), 40 deletions(-) create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_37666/GitHub_37666.cs create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_37666/GitHub_37666.csproj diff --git a/src/coreclr/src/jit/compiler.hpp b/src/coreclr/src/jit/compiler.hpp index 4dc68abbc476a..406562fd7a852 100644 --- a/src/coreclr/src/jit/compiler.hpp +++ b/src/coreclr/src/jit/compiler.hpp @@ -4197,6 +4197,13 @@ bool Compiler::fgVarNeedsExplicitZeroInit(unsigned varNum, bool bbInALoop, bool { LclVarDsc* varDsc = lvaGetDesc(varNum); + if (lvaIsFieldOfDependentlyPromotedStruct(varDsc)) + { + // Fields of dependently promoted structs may only be initialized in the prolog when the whole + // struct is initialized in the prolog. + return fgVarNeedsExplicitZeroInit(varDsc->lvParentLcl, bbInALoop, bbIsReturn); + } + if (bbInALoop && !bbIsReturn) { return true; diff --git a/src/coreclr/src/jit/optimizer.cpp b/src/coreclr/src/jit/optimizer.cpp index 9f4f0c22e758f..3e36d7ab4deeb 100644 --- a/src/coreclr/src/jit/optimizer.cpp +++ b/src/coreclr/src/jit/optimizer.cpp @@ -9224,6 +9224,8 @@ void Compiler::optRemoveRedundantZeroInits() CompAllocator allocator(getAllocator(CMK_ZeroInit)); LclVarRefCounts refCounts(allocator); + BitVecTraits bitVecTraits(lvaCount, this); + BitVec zeroInitLocals = BitVecOps::MakeEmpty(&bitVecTraits); bool hasGCSafePoint = false; bool canThrow = false; @@ -9271,55 +9273,94 @@ void Compiler::optRemoveRedundantZeroInits() case GT_ASG: { GenTreeOp* treeOp = tree->AsOp(); - if (treeOp->gtOp1->OperIs(GT_LCL_VAR, GT_LCL_FLD)) + if (!treeOp->gtOp1->OperIs(GT_LCL_VAR, GT_LCL_FLD)) { - unsigned lclNum = treeOp->gtOp1->AsLclVarCommon()->GetLclNum(); - LclVarDsc* const lclDsc = lvaGetDesc(lclNum); - unsigned* pRefCount = refCounts.LookupPointer(lclNum); - - // pRefCount can't be null because the local node on the lhs of the assignment - // must have already been seen. - assert(pRefCount != nullptr); - if (*pRefCount == 1) + break; + } + + unsigned lclNum = treeOp->gtOp1->AsLclVarCommon()->GetLclNum(); + LclVarDsc* const lclDsc = lvaGetDesc(lclNum); + unsigned* pRefCount = refCounts.LookupPointer(lclNum); + + // pRefCount can't be null because the local node on the lhs of the assignment + // must have already been seen. + assert(pRefCount != nullptr); + if (*pRefCount != 1) + { + break; + } + + unsigned parentRefCount = 0; + if (lclDsc->lvIsStructField && refCounts.Lookup(lclDsc->lvParentLcl, &parentRefCount) && + (parentRefCount != 0)) + { + break; + } + + unsigned fieldRefCount = 0; + if (lclDsc->lvPromoted) + { + for (unsigned i = lclDsc->lvFieldLclStart; + (fieldRefCount == 0) && (i < lclDsc->lvFieldLclStart + lclDsc->lvFieldCnt); ++i) { - // The local hasn't been referenced before this assignment. - bool removedExplicitZeroInit = false; - if (!lclDsc->lvTracked && treeOp->gtOp2->IsIntegralConst(0)) - { - bool bbInALoop = (block->bbFlags & BBF_BACKWARD_JUMP) != 0; - bool bbIsReturn = block->bbJumpKind == BBJ_RETURN; + refCounts.Lookup(i, &fieldRefCount); + } + } - if (!fgVarNeedsExplicitZeroInit(lclNum, bbInALoop, bbIsReturn)) - { - // We are guaranteed to have a zero initialization in the prolog and - // the local hasn't been redefined between the prolog and this explicit - // zero initialization so the assignment can be safely removed. - if (tree == stmt->GetRootNode()) - { - fgRemoveStmt(block, stmt); - removedExplicitZeroInit = true; - *pRefCount = 0; - lclDsc->lvSuppressedZeroInit = 1; - lclDsc->setLvRefCnt(lclDsc->lvRefCnt() - 1); - } - } - } + if (fieldRefCount != 0) + { + break; + } + + // The local hasn't been referenced before this assignment. + bool removedExplicitZeroInit = false; + + if (treeOp->gtOp2->IsIntegralConst(0)) + { + if (!lclDsc->lvTracked) + { + bool bbInALoop = (block->bbFlags & BBF_BACKWARD_JUMP) != 0; + bool bbIsReturn = block->bbJumpKind == BBJ_RETURN; - if (!removedExplicitZeroInit && treeOp->gtOp1->OperIs(GT_LCL_VAR) && - (!canThrow || !lclDsc->lvLiveInOutOfHndlr)) + if (BitVecOps::IsMember(&bitVecTraits, zeroInitLocals, lclNum) || + (lclDsc->lvIsStructField && + BitVecOps::IsMember(&bitVecTraits, zeroInitLocals, lclDsc->lvParentLcl)) || + !fgVarNeedsExplicitZeroInit(lclNum, bbInALoop, bbIsReturn)) { - // If compMethodRequiresPInvokeFrame() returns true, lower may later - // insert a call to CORINFO_HELP_INIT_PINVOKE_FRAME which is a gc-safe point. - if (!lclDsc->HasGCPtr() || - (!GetInterruptible() && !hasGCSafePoint && !compMethodRequiresPInvokeFrame())) + // We are guaranteed to have a zero initialization in the prolog or a + // dominating explicit zero initialization and the local hasn't been redefined + // between the prolog and this explicit zero initialization so the assignment + // can be safely removed. + if (tree == stmt->GetRootNode()) { - // The local hasn't been used and won't be reported to the gc between - // the prolog and this explicit intialization. Therefore, it doesn't - // require zero initialization in the prolog. - lclDsc->lvHasExplicitInit = 1; + fgRemoveStmt(block, stmt); + removedExplicitZeroInit = true; + lclDsc->lvSuppressedZeroInit = 1; + lclDsc->setLvRefCnt(lclDsc->lvRefCnt() - 1); } } } + + if (treeOp->gtOp1->OperIs(GT_LCL_VAR)) + { + BitVecOps::AddElemD(&bitVecTraits, zeroInitLocals, lclNum); + } + *pRefCount = 0; + } + + if (!removedExplicitZeroInit && treeOp->gtOp1->OperIs(GT_LCL_VAR) && + (!canThrow || !lclDsc->lvLiveInOutOfHndlr)) + { + // If compMethodRequiresPInvokeFrame() returns true, lower may later + // insert a call to CORINFO_HELP_INIT_PINVOKE_FRAME which is a gc-safe point. + if (!lclDsc->HasGCPtr() || + (!GetInterruptible() && !hasGCSafePoint && !compMethodRequiresPInvokeFrame())) + { + // The local hasn't been used and won't be reported to the gc between + // the prolog and this explicit intialization. Therefore, it doesn't + // require zero initialization in the prolog. + lclDsc->lvHasExplicitInit = 1; + } } break; } diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_37666/GitHub_37666.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_37666/GitHub_37666.cs new file mode 100644 index 0000000000000..065e12ac1b61c --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_37666/GitHub_37666.cs @@ -0,0 +1,71 @@ +// 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.Runtime.CompilerServices; + +public struct TwoBools +{ + public bool b1; + public bool b2; + + public TwoBools(bool b1, bool b2) + { + this.b1 = b1; + this.b2 = b2; + } +} + +class Test +{ + public static int Main() + { + int result = 100; + + RunTest(Test1, "Test1", ref result); + RunTest(Test2, "Test2", ref result); + + return result; + } + + delegate TwoBools TestZeroInit(); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void RunTest(TestZeroInit test, string testName, ref int result) + { + if (test().b2) + { + Console.WriteLine(testName + " failed"); + result = -1; + } + else + { + Console.WriteLine(testName + " passed"); + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static TwoBools Test1() + { + TwoBools result = CreateTwoBools(); + result.b2 = false; + return result; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static TwoBools Test2() + { + TwoBools result = default(TwoBools); + result.b2 = true; + result = default(TwoBools); + return result; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static TwoBools CreateTwoBools() + { + TwoBools result = new TwoBools(true, true); + return result; + } +} diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_37666/GitHub_37666.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_37666/GitHub_37666.csproj new file mode 100644 index 0000000000000..adc3b7203ae20 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_37666/GitHub_37666.csproj @@ -0,0 +1,13 @@ + + + Exe + 0 + + + None + True + + + + +