Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: fix bug where a gc struct is not zero initialized #67825

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 33 additions & 13 deletions src/coreclr/jit/liveness.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1988,25 +1988,45 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
if (blockRange.TryGetUse(node, &addrUse) &&
(addrUse.User()->OperIs(GT_STOREIND, GT_STORE_BLK, GT_STORE_OBJ)))
{
// Remove the store. DCE will iteratively clean up any ununsed operands.
GenTreeIndir* const store = addrUse.User()->AsIndir();

JITDUMP("Removing dead indirect store:\n");
DISPNODE(store);
// If this is a zero init of an explicit zero init gc local
// that has at least one other reference, we will keep the zero init.
//
Comment on lines +1993 to +1995
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe expand a bit on why? This just seems to be a restatement of the code below.

const LclVarDsc& varDsc = lvaTable[node->AsLclVarCommon()->GetLclNum()];
const bool isExplicitInitLocal = varDsc.lvHasExplicitInit;
const bool isReferencedLocal = varDsc.lvRefCnt() > 1;
const bool isZeroInit = store->OperIsInitBlkOp();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you also need to check the init value is actually zero if you only want to avoid deleting only zero inits? E.g., check IsConstInitVal and then the constant init value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this problem occur for normal untracked ref/byref lclVar and not just for struct field gc refs reported as untracked to the gc?

i.e., should we stop marking all gc vars or structs with gc fields as explicit init in optRemoveRedundantZeroInits?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you also need to check the init value is actually zero

I suppose so, but this kicks in so rarely it won't matter in practice.

Couldn't this problem occur for normal untracked ref/byref lclVa

We won't have liveness info for these so won't dead store.

const bool isGCInit = varDsc.HasGCPtr();

assert(store->Addr() == node);
blockRange.Delete(this, block, node);

GenTree* data =
store->OperIs(GT_STOREIND) ? store->AsStoreInd()->Data() : store->AsBlk()->Data();
data->SetUnusedValue();

if (data->isIndir())
if (isExplicitInitLocal && isReferencedLocal && isZeroInit && isGCInit)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know if this is the first dead store. E.g., what if the lclVar has multiple zeroing that are dead stores? All of them will be kept even though (probably) only the first was the "explicit init" one as determined by optRemoveRedundantZeroInits.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, if there are multiple dead stores we will keep them all.

{
Lowering::TransformUnusedIndirection(data->AsIndir(), this, block);
// Yes, we'd better keep it around.
//
JITDUMP("Keeping dead indirect store -- explicit zero init of gc type\n");
DISPNODE(store);
}
else
{
// Remove the store. DCE will iteratively clean up any ununsed operands.
//
JITDUMP("Removing dead indirect store:\n");
DISPNODE(store);

assert(store->Addr() == node);
blockRange.Delete(this, block, node);

fgRemoveDeadStoreLIR(store, block);
GenTree* data =
store->OperIs(GT_STOREIND) ? store->AsStoreInd()->Data() : store->AsBlk()->Data();
data->SetUnusedValue();

if (data->isIndir())
{
Lowering::TransformUnusedIndirection(data->AsIndir(), this, block);
}

fgRemoveDeadStoreLIR(store, block);
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/optimizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9492,7 +9492,7 @@ void Compiler::optRemoveRedundantZeroInits()
// the prolog and this explicit intialization. Therefore, it doesn't
// require zero initialization in the prolog.
lclDsc->lvHasExplicitInit = 1;
JITDUMP("Marking " FMT_LP " as having an explicit init\n", lclNum);
JITDUMP("Marking V%02u as having an explicit init\n", lclNum);
}
}
break;
Expand Down
73 changes: 73 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// 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.Collections.Generic;
using System.Runtime.CompilerServices;

public struct Key
{
public int a;
public string s;
}

public struct Problem
{
public int x;
public double d;
public string s0;
public int y;
public double e;
public string s1;
}

public class Runtime_65694
{
public Dictionary<Key, Problem> _d;

[MethodImpl(MethodImplOptions.NoInlining)]
public void D()
{
Problem p = new Problem { s0 = "hello", s1 = "world", x = 33 };
Key k = new Key() { a = 0, s = "a" };
Dictionary<Key, Problem> d = new Dictionary<Key, Problem>();
d[k] = p;

_d = d;
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static void F()
{
GC.Collect();
}

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
public int G(Key k, bool b)
{
Problem p = default;

F();

if (b)
{
if (_d?.TryGetValue(k, out p) == true && (p.x == 33))
{
return 22;
}
}

return 0;
}

public static int Main()
{
var r = new Runtime_65694();
r.D();
int result = 0;
Key k = new Key() { a = 0, s = "a" };
result += r.G(k, true);
return result + 78;
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>