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

Expand the GC hole fix for explicitly initialized structs #69501

Merged
merged 2 commits into from
May 18, 2022
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
3 changes: 2 additions & 1 deletion src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -4590,7 +4590,8 @@ class Compiler

bool fgTryRemoveNonLocal(GenTree* node, LIR::Range* blockRange);

void fgRemoveDeadStoreLIR(GenTree* store, BasicBlock* block);
bool fgTryRemoveDeadStoreLIR(GenTree* store, GenTreeLclVarCommon* lclNode, BasicBlock* block);

bool fgRemoveDeadStore(GenTree** pTree,
LclVarDsc* varDsc,
VARSET_VALARG_TP life,
Expand Down
12 changes: 12 additions & 0 deletions src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -3280,6 +3280,12 @@ struct GenTreeLclVarCommon : public GenTreeUnOp
SetLclNum(lclNum);
}

GenTree*& Data()
{
assert(OperIsLocalStore());
return gtOp1;
}

unsigned GetLclNum() const
{
return _gtLclNum;
Expand Down Expand Up @@ -6602,6 +6608,12 @@ struct GenTreeIndir : public GenTreeOp
gtOp1 = addr;
}

GenTree*& Data()
{
assert(OperIs(GT_STOREIND) || OperIsStoreBlk());
return gtOp2;
}

// these methods provide an interface to the indirection node which
bool HasBase();
bool HasIndex();
Expand Down
80 changes: 37 additions & 43 deletions src/coreclr/jit/liveness.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1970,42 +1970,19 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
{
GenTreeIndir* const store = addrUse.User()->AsIndir();

// 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.
//
const LclVarDsc& varDsc = lvaTable[node->AsLclVarCommon()->GetLclNum()];
const bool isExplicitInitLocal = varDsc.lvHasExplicitInit;
const bool isReferencedLocal = varDsc.lvRefCnt() > 1;
const bool isZeroInit = store->OperIsInitBlkOp();
const bool isGCInit = varDsc.HasGCPtr();

if (isExplicitInitLocal && isReferencedLocal && isZeroInit && isGCInit)
if (fgTryRemoveDeadStoreLIR(store, node->AsLclVarCommon(), 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);
JITDUMP("Removing dead LclVar address:\n");
DISPNODE(node);
blockRange.Remove(node);

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

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

fgRemoveDeadStoreLIR(store, block);
}
}
}
Expand All @@ -2027,17 +2004,10 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
isDeadStore = fgComputeLifeUntrackedLocal(life, keepAliveVars, varDsc, lclVarNode);
}

if (isDeadStore)
if (isDeadStore && fgTryRemoveDeadStoreLIR(node, lclVarNode, block))
{
JITDUMP("Removing dead store:\n");
DISPNODE(lclVarNode);

// Remove the store. DCE will iteratively clean up any ununsed operands.
lclVarNode->gtOp1->SetUnusedValue();

fgRemoveDeadStoreLIR(node, block);
lclVarNode->Data()->SetUnusedValue();
}

break;
}

Expand Down Expand Up @@ -2185,17 +2155,41 @@ bool Compiler::fgTryRemoveNonLocal(GenTree* node, LIR::Range* blockRange)
}

//---------------------------------------------------------------------
// fgRemoveDeadStoreLIR - remove a dead store from LIR
// fgTryRemoveDeadStoreLIR - try to remove a dead store from LIR
//
// store - A store tree
// block - Block that the store is part of
// Arguments:
// store - A store tree
// lclNode - The node representing the local being stored to
// block - Block that the store is part of
//
// Return Value:
// Whether the store was successfully removed from "block"'s range.
//
void Compiler::fgRemoveDeadStoreLIR(GenTree* store, BasicBlock* block)
bool Compiler::fgTryRemoveDeadStoreLIR(GenTree* store, GenTreeLclVarCommon* lclNode, BasicBlock* block)
{
LIR::Range& blockRange = LIR::AsRange(block);
blockRange.Remove(store);
assert(!opts.MinOpts());

// We cannot remove stores to (tracked) TYP_STRUCT locals with GC pointers marked as "explicit init",
// as said locals will be reported to the GC untracked, and deleting the explicit initializer risks
// exposing uninitialized references.
if ((lclNode->gtFlags & GTF_VAR_USEASG) == 0)
{
LclVarDsc* varDsc = lvaGetDesc(lclNode);
if (varDsc->lvHasExplicitInit && (varDsc->TypeGet() == TYP_STRUCT) && varDsc->HasGCPtr() &&
(varDsc->lvRefCnt() > 1))
{
JITDUMP("Not removing a potential explicit init [%06u] of V%02u\n", dspTreeID(store), lclNode->GetLclNum());
return false;
}
}

JITDUMP("Removing dead %s:\n", store->OperIsIndir() ? "indirect store" : "local store");
DISPNODE(store);

LIR::AsRange(block).Remove(store);
fgStmtRemoved = true;

return true;
}

//---------------------------------------------------------------------
Expand Down
41 changes: 41 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_65694/Runtime_65694_2.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// 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;

public class Runtime_65694_2
{
public static int Main()
{
var a = new StructWithObj { Obj = new object() };
var c = new StructWithObj { Obj = new object() };

return Problem(a, c).Obj == c.Obj ? 100 : 101;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static StructWithObj Problem(StructWithObj a, StructWithObj c)
{
StructWithObj b = a;

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

GcSafePoint();
GcSafePoint();

if (a.Obj == b.Obj)
{
b = c;
}

return b;
}

struct StructWithObj
{
public object Obj;
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
<CLRTestBatchPreCommands><![CDATA[
$(CLRTestBatchPreCommands)
set DOTNET_JitNoStructPromotion=1
]]></CLRTestBatchPreCommands>
<BashCLRTestPreCommands><![CDATA[
$(BashCLRTestPreCommands)
export DOTNET_JitNoStructPromotion=1
]]></BashCLRTestPreCommands>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>