Skip to content

Commit

Permalink
Always unroll GT_STORE_BLK for blocks with gc refs on heap
Browse files Browse the repository at this point in the history
  • Loading branch information
EgorBo committed Dec 1, 2023
1 parent 3e55167 commit 878f932
Show file tree
Hide file tree
Showing 7 changed files with 85 additions and 8 deletions.
11 changes: 9 additions & 2 deletions src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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);
}

Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/jit/lower.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down
10 changes: 8 additions & 2 deletions src/coreclr/jit/lowerarmarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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
Expand All @@ -610,6 +614,8 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
}
else
{
// CI Test
assert(!blkNode->ContainsReferences());
blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindHelper;
}
}
Expand Down
5 changes: 3 additions & 2 deletions src/coreclr/jit/lowerloongarch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
15 changes: 13 additions & 2 deletions src/coreclr/jit/lowerxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;
Expand Down
31 changes: 31 additions & 0 deletions src/tests/JIT/opt/Structs/StructWithGC_Zeroing.cs
Original file line number Diff line number Diff line change
@@ -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;
}
}
14 changes: 14 additions & 0 deletions src/tests/JIT/opt/Structs/StructWithGC_Zeroing.csproj
Original file line number Diff line number Diff line change
@@ -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>

0 comments on commit 878f932

Please sign in to comment.