Always unroll GT_STORE_BLK for blocks with gc refs on heap

Author: EgorBo

src/coreclr/jit/gentree.h

@@ -7337,12 +7337,15 @@ struct GenTreeBlk : public GenTreeIndir
     bool gtBlkOpGcUnsafe;
 #endif
 
-#ifdef TARGET_XARCH
     bool IsOnHeapAndContainsReferences()
     {
         return (m_layout != nullptr) && m_layout->HasGCPtr() && !Addr()->OperIs(GT_LCL_ADDR);
     }
-#endif
+
+    bool ContainsReferences()
+    {
+        return (m_layout != nullptr) && m_layout->HasGCPtr();
+    }
 
     GenTreeBlk(genTreeOps oper, var_types type, GenTree* addr, ClassLayout* layout)
         : GenTreeIndir(oper, type, addr, nullptr)
@@ -7353,6 +7356,10 @@ struct GenTreeBlk : public GenTreeIndir
     GenTreeBlk(genTreeOps oper, var_types type, GenTree* addr, GenTree* data, ClassLayout* layout)
         : GenTreeIndir(oper, type, addr, data)
     {
+        if (data->IsIntegralConst(0))
+        {
+            data->gtFlags |= GTF_DONT_CSE;
+        }
         Initialize(layout);
     }
 

src/coreclr/jit/lower.cpp

@@ -8867,6 +8867,13 @@ void Lowering::LowerBlockStoreCommon(GenTreeBlk* blkNode)
 {
     assert(blkNode->OperIs(GT_STORE_BLK, GT_STORE_DYN_BLK));
 
+    if (blkNode->OperIs(GT_STORE_DYN_BLK))
+    {
+        // CI Test: make sure we don't use GT_STORE_DYN_BLK
+        // for blocks with GC references.
+        assert(!blkNode->ContainsReferences());
+    }
+
     // Lose the type information stored in the source - we no longer need it.
     if (blkNode->Data()->OperIs(GT_BLK))
     {

src/coreclr/jit/lowerarmarch.cpp

@@ -570,8 +570,9 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
             src = src->AsUnOp()->gtGetOp1();
         }
 
-        if (!blkNode->OperIs(GT_STORE_DYN_BLK) && (size <= comp->getUnrollThreshold(Compiler::UnrollKind::Memset)) &&
-            src->OperIs(GT_CNS_INT))
+        const bool shouldBeUnrolled = blkNode->IsOnHeapAndContainsReferences() ||
+                                      (size <= comp->getUnrollThreshold(Compiler::UnrollKind::Memset));
+        if (!blkNode->OperIs(GT_STORE_DYN_BLK) && shouldBeUnrolled && src->OperIs(GT_CNS_INT))
         {
             blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindUnroll;
 
@@ -584,6 +585,9 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
 
             ssize_t fill = src->AsIntCon()->IconValue() & 0xFF;
 
+            // CI Test: it's a bad idea to use non-zero here
+            assert((fill == 0) || !blkNode->ContainsReferences());
+
             if (fill == 0)
             {
 #ifdef TARGET_ARM64
@@ -610,6 +614,8 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
         }
         else
         {
+            // CI Test
+            assert(!blkNode->ContainsReferences());
             blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindHelper;
         }
     }

src/coreclr/jit/lowerloongarch64.cpp

@@ -296,8 +296,9 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
             src = src->AsUnOp()->gtGetOp1();
         }
 
-        if (!blkNode->OperIs(GT_STORE_DYN_BLK) && (size <= comp->getUnrollThreshold(Compiler::UnrollKind::Memset)) &&
-            src->OperIs(GT_CNS_INT))
+        const bool shouldBeUnrolled = blkNode->IsOnHeapAndContainsReferences() ||
+                                      (size <= comp->getUnrollThreshold(Compiler::UnrollKind::Memset));
+        if (!blkNode->OperIs(GT_STORE_DYN_BLK) && shouldBeUnrolled && src->OperIs(GT_CNS_INT))
         {
             blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindUnroll;
 

src/coreclr/jit/lowerxarch.cpp

@@ -338,10 +338,15 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
             src = src->AsUnOp()->gtGetOp1();
         }
 
-        if (!blkNode->OperIs(GT_STORE_DYN_BLK) && (size <= comp->getUnrollThreshold(Compiler::UnrollKind::Memset)))
+        const bool shouldBeUnrolled = blkNode->IsOnHeapAndContainsReferences() ||
+                                      (size <= comp->getUnrollThreshold(Compiler::UnrollKind::Memset));
+        if (!blkNode->OperIs(GT_STORE_DYN_BLK) && shouldBeUnrolled)
         {
             if (!src->OperIs(GT_CNS_INT))
             {
+                // CI Test: Hopefully, we won't see anything but GT_CNS_INT(0) here for blocks with GC pointers.
+                assert(!blkNode->ContainsReferences());
+
                 // TODO-CQ: We could unroll even when the initialization value is not a constant
                 // by inserting a MUL init, 0x01010101 instruction. We need to determine if the
                 // extra latency that MUL introduces isn't worse that rep stosb. Likely not.
@@ -357,9 +362,15 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
                 // the largest width store of the desired inline expansion.
 
                 ssize_t fill = src->AsIntCon()->IconValue() & 0xFF;
+                if (blkNode->ContainsReferences())
+                {
+                    // CI Test: it's a bad idea to use non-zero here
+                    assert(fill == 0);
+                }
 
                 const bool canUseSimd = !blkNode->IsOnHeapAndContainsReferences() && comp->IsBaselineSimdIsaSupported();
-                if (size > comp->getUnrollThreshold(Compiler::UnrollKind::Memset, canUseSimd))
+                if (!blkNode->IsOnHeapAndContainsReferences() &&
+                    (size > comp->getUnrollThreshold(Compiler::UnrollKind::Memset, canUseSimd)))
                 {
                     // It turns out we can't use SIMD so the default threshold is too big
                     goto TOO_BIG_TO_UNROLL;

src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs

@@ -0,0 +1,31 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.CompilerServices;
+using Xunit;
+
+public class StructWithGC_Zeroing
+{
+    [Fact]
+    public static void StructZeroingShouldNotUseMemset()
+    {
+        var largeStructWithGc = new LargeStructWithGC();
+        Test(ref largeStructWithGc);
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static void Test(ref LargeStructWithGC s)
+    {
+        // X64-NOT: CORINFO_HELP_MEMSET
+        // ARM64-NOT: CORINFO_HELP_MEMSET
+        s = default;
+    }
+
+    struct LargeStructWithGC
+    {
+        public byte x;
+        public string a;
+        public long b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, r, s, t, u, v, w, z;
+    }
+}

src/tests/JIT/opt/Structs/StructWithGC_Zeroing.csproj

@@ -0,0 +1,14 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <RequiresProcessIsolation>true</RequiresProcessIsolation>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs">
+      <HasDisasmCheck>true</HasDisasmCheck>
+    </Compile>
+  </ItemGroup>
+</Project>