Fixes and improvements for removal of redundant zero inits (dotnet#37786

)

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 dotnet#37666.

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;

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;
                     }

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;
+    }
+}

src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_37666/GitHub_37666.csproj

@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <CLRTestPriority>0</CLRTestPriority>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>