From ded470a688e419b114a3f9b249c4c1b738d59f42 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 18 Jan 2025 21:39:14 +0100 Subject: [PATCH 1/4] Remove more write-barriers around byreflike structs --- src/coreclr/jit/assertionprop.cpp | 24 +++++++++++++++--------- src/coreclr/jit/codegenarm.cpp | 3 +-- src/coreclr/jit/codegenarm64.cpp | 3 +-- src/coreclr/jit/codegenloongarch64.cpp | 3 +-- src/coreclr/jit/codegenriscv64.cpp | 3 +-- src/coreclr/jit/codegenxarch.cpp | 3 +-- src/coreclr/jit/gentree.cpp | 20 ++++++++++++++++++++ src/coreclr/jit/gentree.h | 1 + src/coreclr/jit/importer.cpp | 9 ++++++++- src/coreclr/jit/lowerarmarch.cpp | 2 +- src/coreclr/jit/lowerloongarch64.cpp | 2 +- src/coreclr/jit/lowerriscv64.cpp | 2 +- src/coreclr/jit/lowerxarch.cpp | 4 ++-- src/coreclr/jit/morphblock.cpp | 9 +++++++-- 14 files changed, 61 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 51220e99b352eb..a000403937de55 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -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; @@ -5250,18 +5251,23 @@ 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(funcApp.m_args[0]); + if ((lclNum == comp->info.compRetBuffArg) && + comp->eeIsByrefLike(comp->info.compMethodInfo->args.retTypeClass)) + { + return GCInfo::WriteBarrierForm::WBF_NoBarrier; } } } - - // TODO: - // * addr is ByRefLike - NoBarrier (https://github.com/dotnet/runtime/issues/9512) - // return GCInfo::WriteBarrierForm::WBF_BarrierUnknown; } @@ -5315,7 +5321,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)); diff --git a/src/coreclr/jit/codegenarm.cpp b/src/coreclr/jit/codegenarm.cpp index 80d0e05ba1c12d..92d6bc8635224e 100644 --- a/src/coreclr/jit/codegenarm.cpp +++ b/src/coreclr/jit/codegenarm.cpp @@ -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()); diff --git a/src/coreclr/jit/codegenarm64.cpp b/src/coreclr/jit/codegenarm64.cpp index ab8d95125ff5c8..6e4db910847df7 100644 --- a/src/coreclr/jit/codegenarm64.cpp +++ b/src/coreclr/jit/codegenarm64.cpp @@ -3665,8 +3665,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()); diff --git a/src/coreclr/jit/codegenloongarch64.cpp b/src/coreclr/jit/codegenloongarch64.cpp index 7f235ee2a1d1d6..f8e26e956209aa 100644 --- a/src/coreclr/jit/codegenloongarch64.cpp +++ b/src/coreclr/jit/codegenloongarch64.cpp @@ -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()); diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 9ce76ae3c0dab9..8efe6e0827125c 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -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()); diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index b3090f94e2c4f9..750b2d9818ba73 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -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. diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index ba5407c7d02886..73ec38f460e263 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -18244,6 +18244,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; diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index d5b7b6066afe9b..648f0c99abee2b 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -7941,6 +7941,7 @@ struct GenTreeIndir : public GenTreeOp return gtOp2; } + bool IsAddressNotOnHeap(Compiler* comp); bool HasBase(); bool HasIndex(); GenTree* Base(); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index a7fe7280946dda..5bd6e842daa119 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -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; } diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index a5b982ab61af83..cc02a343cf67ed 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -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 diff --git a/src/coreclr/jit/lowerloongarch64.cpp b/src/coreclr/jit/lowerloongarch64.cpp index 72435464dbb099..ef06d2ce41791b 100644 --- a/src/coreclr/jit/lowerloongarch64.cpp +++ b/src/coreclr/jit/lowerloongarch64.cpp @@ -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 diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 55c4182430461d..32302506ae5579 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -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 diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index c80d19e9fdd4f2..8b38842d8e9fc0 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -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 @@ -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(); diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index e1f0baf084fc2f..4b81ca7d79eaa3 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -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); } } } From 7dccd405a114ff608720608a530f6f9b9aad55a0 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 18 Jan 2025 22:36:33 +0100 Subject: [PATCH 2/4] check compMethodHasRetVal --- src/coreclr/jit/assertionprop.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index a000403937de55..0fdbbaf7c35f7e 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -5258,7 +5258,7 @@ static GCInfo::WriteBarrierForm GetWriteBarrierForm(Compiler* comp, ValueNum vn) return GetWriteBarrierForm(comp, funcApp.m_args[0]); } } - if (funcApp.m_func == VNF_InitVal) + if ((funcApp.m_func == VNF_InitVal) && comp->compMethodHasRetVal()) { unsigned lclNum = vnStore->CoercedConstantValue(funcApp.m_args[0]); if ((lclNum == comp->info.compRetBuffArg) && From 8ea6336f02ee9ebc6eb2d64070d65c45892a1798 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 19 Jan 2025 00:04:16 +0100 Subject: [PATCH 3/4] add 'this' --- src/coreclr/jit/assertionprop.cpp | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 0fdbbaf7c35f7e..e151bbb2f23ff1 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -5258,13 +5258,30 @@ static GCInfo::WriteBarrierForm GetWriteBarrierForm(Compiler* comp, ValueNum vn) return GetWriteBarrierForm(comp, funcApp.m_args[0]); } } - if ((funcApp.m_func == VNF_InitVal) && comp->compMethodHasRetVal()) + if (funcApp.m_func == VNF_InitVal) { - unsigned lclNum = vnStore->CoercedConstantValue(funcApp.m_args[0]); - if ((lclNum == comp->info.compRetBuffArg) && - comp->eeIsByrefLike(comp->info.compMethodInfo->args.retTypeClass)) + // See if the address is in current method's return buffer + // while the return type is a byref-like type. + if (comp->compMethodHasRetVal()) { - return GCInfo::WriteBarrierForm::WBF_NoBarrier; + unsigned lclNum = vnStore->CoercedConstantValue(funcApp.m_args[0]); + if ((lclNum == comp->info.compRetBuffArg) && + comp->eeIsByrefLike(comp->info.compMethodInfo->args.retTypeClass)) + { + return GCInfo::WriteBarrierForm::WBF_NoBarrier; + } + } + + // Same for implicit "this" parameter + if ((comp->info.compThisArg != BAD_VAR_NUM)) + { + assert(!comp->info.compIsStatic); + + unsigned lclNum = vnStore->CoercedConstantValue(funcApp.m_args[0]); + if ((lclNum == comp->info.compThisArg) && comp->eeIsByrefLike(comp->info.compClassHnd)) + { + return GCInfo::WriteBarrierForm::WBF_NoBarrier; + } } } } From ff735d0ce2b567d24558b95b2b4bfcab27a0de44 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 20 Jan 2025 14:17:21 +0100 Subject: [PATCH 4/4] Refactor assertion propagation logic in assertionprop.cpp --- src/coreclr/jit/assertionprop.cpp | 32 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index e151bbb2f23ff1..288891b0daab44 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -5260,28 +5260,26 @@ static GCInfo::WriteBarrierForm GetWriteBarrierForm(Compiler* comp, ValueNum vn) } if (funcApp.m_func == VNF_InitVal) { - // See if the address is in current method's return buffer - // while the return type is a byref-like type. - if (comp->compMethodHasRetVal()) + unsigned lclNum = vnStore->CoercedConstantValue(funcApp.m_args[0]); + assert(lclNum != BAD_VAR_NUM); + CORINFO_CLASS_HANDLE srcCls = NO_CLASS_HANDLE; + + if (comp->compMethodHasRetVal() && (lclNum == comp->info.compRetBuffArg)) { - unsigned lclNum = vnStore->CoercedConstantValue(funcApp.m_args[0]); - if ((lclNum == comp->info.compRetBuffArg) && - comp->eeIsByrefLike(comp->info.compMethodInfo->args.retTypeClass)) - { - return GCInfo::WriteBarrierForm::WBF_NoBarrier; - } + // 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; } - - // Same for implicit "this" parameter - if ((comp->info.compThisArg != BAD_VAR_NUM)) + else if (lclNum == comp->info.compThisArg) { + // Same for implicit "this" parameter assert(!comp->info.compIsStatic); + srcCls = comp->info.compClassHnd; + } - unsigned lclNum = vnStore->CoercedConstantValue(funcApp.m_args[0]); - if ((lclNum == comp->info.compThisArg) && comp->eeIsByrefLike(comp->info.compClassHnd)) - { - return GCInfo::WriteBarrierForm::WBF_NoBarrier; - } + if ((srcCls != NO_CLASS_HANDLE) && comp->eeIsByrefLike(srcCls)) + { + return GCInfo::WriteBarrierForm::WBF_NoBarrier; } } }