Skip to content

Commit

Permalink
Remove more write-barriers around byreflike structs (#111576)
Browse files Browse the repository at this point in the history
  • Loading branch information
EgorBo authored Jan 22, 2025
1 parent 9031940 commit 00c8751
Show file tree
Hide file tree
Showing 14 changed files with 76 additions and 27 deletions.
39 changes: 30 additions & 9 deletions src/coreclr/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5207,9 +5207,10 @@ bool Compiler::optNonNullAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree*
// Return Value:
// Exact type of write barrier required for the given address.
//
static GCInfo::WriteBarrierForm GetWriteBarrierForm(ValueNumStore* vnStore, ValueNum vn)
static GCInfo::WriteBarrierForm GetWriteBarrierForm(Compiler* comp, ValueNum vn)
{
const var_types type = vnStore->TypeOfVN(vn);
ValueNumStore* vnStore = comp->vnStore;
const var_types type = vnStore->TypeOfVN(vn);
if (type == TYP_REF)
{
return GCInfo::WriteBarrierForm::WBF_BarrierUnchecked;
Expand Down Expand Up @@ -5250,18 +5251,38 @@ static GCInfo::WriteBarrierForm GetWriteBarrierForm(ValueNumStore* vnStore, Valu
//
if (vnStore->IsVNConstantNonHandle(funcApp.m_args[0]))
{
return GetWriteBarrierForm(vnStore, funcApp.m_args[1]);
return GetWriteBarrierForm(comp, funcApp.m_args[1]);
}
if (vnStore->IsVNConstantNonHandle(funcApp.m_args[1]))
{
return GetWriteBarrierForm(vnStore, funcApp.m_args[0]);
return GetWriteBarrierForm(comp, funcApp.m_args[0]);
}
}
}
if (funcApp.m_func == VNF_InitVal)
{
unsigned lclNum = vnStore->CoercedConstantValue<unsigned>(funcApp.m_args[0]);
assert(lclNum != BAD_VAR_NUM);
CORINFO_CLASS_HANDLE srcCls = NO_CLASS_HANDLE;

// TODO:
// * addr is ByRefLike - NoBarrier (https://github.com/dotnet/runtime/issues/9512)
//
if (comp->compMethodHasRetVal() && (lclNum == comp->info.compRetBuffArg))
{
// See if the address is in current method's return buffer
// while the return type is a byref-like type.
srcCls = comp->info.compMethodInfo->args.retTypeClass;
}
else if (lclNum == comp->info.compThisArg)
{
// Same for implicit "this" parameter
assert(!comp->info.compIsStatic);
srcCls = comp->info.compClassHnd;
}

if ((srcCls != NO_CLASS_HANDLE) && comp->eeIsByrefLike(srcCls))
{
return GCInfo::WriteBarrierForm::WBF_NoBarrier;
}
}
}
return GCInfo::WriteBarrierForm::WBF_BarrierUnknown;
}

Expand Down Expand Up @@ -5315,7 +5336,7 @@ bool Compiler::optWriteBarrierAssertionProp_StoreInd(ASSERT_VALARG_TP assertions
{
// NOTE: we might want to inspect indirs with GTF_IND_TGT_HEAP flag as well - what if we can prove
// that they actually need no barrier? But that comes with a TP regression.
barrierType = GetWriteBarrierForm(vnStore, addr->gtVNPair.GetConservative());
barrierType = GetWriteBarrierForm(this, addr->gtVNPair.GetConservative());
}

JITDUMP("Trying to determine the exact type of write barrier for STOREIND [%d06]: ", dspTreeID(indir));
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/codegenarm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -811,8 +811,7 @@ void CodeGen::genCodeForCpObj(GenTreeBlk* cpObjNode)
sourceIsLocal = true;
}

bool dstOnStack =
dstAddr->gtSkipReloadOrCopy()->OperIs(GT_LCL_ADDR) || cpObjNode->GetLayout()->IsStackOnly(compiler);
bool dstOnStack = cpObjNode->IsAddressNotOnHeap(compiler);

#ifdef DEBUG
assert(!dstAddr->isContained());
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/codegenarm64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3695,8 +3695,7 @@ void CodeGen::genCodeForCpObj(GenTreeBlk* cpObjNode)
sourceIsLocal = true;
}

bool dstOnStack =
dstAddr->gtSkipReloadOrCopy()->OperIs(GT_LCL_ADDR) || cpObjNode->GetLayout()->IsStackOnly(compiler);
bool dstOnStack = cpObjNode->IsAddressNotOnHeap(compiler);

#ifdef DEBUG
assert(!dstAddr->isContained());
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/codegenloongarch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2233,8 +2233,7 @@ void CodeGen::genCodeForCpObj(GenTreeBlk* cpObjNode)
sourceIsLocal = true;
}

bool dstOnStack =
dstAddr->gtSkipReloadOrCopy()->OperIs(GT_LCL_ADDR) || cpObjNode->GetLayout()->IsStackOnly(compiler);
bool dstOnStack = cpObjNode->IsAddressNotOnHeap(compiler);

#ifdef DEBUG
assert(!dstAddr->isContained());
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/codegenriscv64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2153,8 +2153,7 @@ void CodeGen::genCodeForCpObj(GenTreeBlk* cpObjNode)
sourceIsLocal = true;
}

bool dstOnStack =
dstAddr->gtSkipReloadOrCopy()->OperIs(GT_LCL_ADDR) || cpObjNode->GetLayout()->IsStackOnly(compiler);
bool dstOnStack = cpObjNode->IsAddressNotOnHeap(compiler);

#ifdef DEBUG
assert(!dstAddr->isContained());
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/codegenxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4218,8 +4218,7 @@ void CodeGen::genCodeForCpObj(GenTreeBlk* cpObjNode)
GenTree* dstAddr = cpObjNode->Addr();
GenTree* source = cpObjNode->Data();
var_types srcAddrType = TYP_BYREF;
bool dstOnStack =
dstAddr->gtSkipReloadOrCopy()->OperIs(GT_LCL_ADDR) || cpObjNode->GetLayout()->IsStackOnly(compiler);
bool dstOnStack = cpObjNode->IsAddressNotOnHeap(compiler);

// If the GenTree node has data about GC pointers, this means we're dealing
// with CpObj, so this requires special logic.
Expand Down
20 changes: 20 additions & 0 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18246,6 +18246,26 @@ bool GenTree::isIndir() const
return OperGet() == GT_IND || OperGet() == GT_STOREIND;
}

bool GenTreeIndir::IsAddressNotOnHeap(Compiler* comp)
{
if (OperIs(GT_STOREIND, GT_STORE_BLK) && ((gtFlags & GTF_IND_TGT_NOT_HEAP) != 0))
{
return true;
}

if (HasBase() && Base()->gtSkipReloadOrCopy()->OperIs(GT_LCL_ADDR))
{
return true;
}

if (OperIs(GT_STORE_BLK) && AsBlk()->GetLayout()->IsStackOnly(comp))
{
return true;
}

return false;
}

bool GenTreeIndir::HasBase()
{
return Base() != nullptr;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -7943,6 +7943,7 @@ struct GenTreeIndir : public GenTreeOp
return gtOp2;
}

bool IsAddressNotOnHeap(Compiler* comp);
bool HasBase();
bool HasIndex();
GenTree* Base();
Expand Down
9 changes: 8 additions & 1 deletion src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10607,7 +10607,14 @@ void Compiler::impImportBlockCode(BasicBlock* block)
op1 = impPopStack().val; // Ptr
assertImp(varTypeIsStruct(op2));

op1 = gtNewStoreValueNode(layout, op1, op2, impPrefixFlagsToIndirFlags(prefixFlags));
GenTreeFlags indirFlags = impPrefixFlagsToIndirFlags(prefixFlags);
if (eeIsByrefLike(resolvedToken.hClass))
{
indirFlags |= GTF_IND_TGT_NOT_HEAP;
}

op1 = gtNewStoreValueNode(layout, op1, op2, indirFlags);

op1 = impStoreStruct(op1, CHECK_SPILL_ALL);
goto SPILL_APPEND;
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/lowerarmarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -762,7 +762,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
{
// No write barriers are needed on the stack.
// If the layout contains a byref, then we know it must live on the stack.
if (dstAddr->OperIs(GT_LCL_ADDR) || layout->IsStackOnly(comp))
if (blkNode->IsAddressNotOnHeap(comp))
{
// If the size is small enough to unroll then we need to mark the block as non-interruptible
// to actually allow unrolling. The generated code does not report GC references loaded in the
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/lowerloongarch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
{
// No write barriers are needed on the stack.
// If the layout contains a byref, then we know it must live on the stack.
if (dstAddr->OperIs(GT_LCL_ADDR) || layout->IsStackOnly(comp))
if (blkNode->IsAddressNotOnHeap(comp))
{
// If the size is small enough to unroll then we need to mark the block as non-interruptible
// to actually allow unrolling. The generated code does not report GC references loaded in the
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/lowerriscv64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
{
// No write barriers are needed on the stack.
// If the layout contains a byref, then we know it must live on the stack.
if (dstAddr->OperIs(GT_LCL_ADDR) || layout->IsStackOnly(comp))
if (blkNode->IsAddressNotOnHeap(comp))
{
// If the size is small enough to unroll then we need to mark the block as non-interruptible
// to actually allow unrolling. The generated code does not report GC references loaded in the
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/lowerxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
{
// No write barriers are needed on the stack.
// If the layout is byref-like, then we know it must live on the stack.
if (dstAddr->OperIs(GT_LCL_ADDR) || layout->IsStackOnly(comp))
if (blkNode->IsAddressNotOnHeap(comp))
{
// If the size is small enough to unroll then we need to mark the block as non-interruptible
// to actually allow unrolling. The generated code does not report GC references loaded in the
Expand All @@ -477,7 +477,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
// the entire operation takes 20 cycles and encodes in 5 bytes (loading RCX and REP MOVSD/Q).
unsigned nonGCSlots = 0;

if (dstAddr->OperIs(GT_LCL_ADDR) || layout->IsStackOnly(comp))
if (blkNode->IsAddressNotOnHeap(comp))
{
// If the destination is on the stack then no write barriers are needed.
nonGCSlots = layout->GetSlotCount();
Expand Down
9 changes: 7 additions & 2 deletions src/coreclr/jit/morphblock.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1358,8 +1358,13 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField()
}
else
{
GenTree* fldAddr = grabAddr(srcFieldOffset);
dstFldStore = m_comp->gtNewStoreIndNode(srcType, fldAddr, srcFld);
GenTree* fldAddr = grabAddr(srcFieldOffset);
GenTreeFlags indirFlags = GTF_EMPTY;
if (m_store->OperIs(GT_STORE_BLK, GT_STOREIND))
{
indirFlags = m_store->gtFlags & (GTF_IND_TGT_NOT_HEAP | GTF_IND_TGT_HEAP);
}
dstFldStore = m_comp->gtNewStoreIndNode(srcType, fldAddr, srcFld, indirFlags);
}
}
}
Expand Down

0 comments on commit 00c8751

Please sign in to comment.