From 5f91d491c8d548dfa3779f5d2fa18068762bf84b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 5 Feb 2024 01:07:12 +0100 Subject: [PATCH 1/6] Mark indirs with GTF_IND_TGT_NOT_HEAP and GTF_IND_TGT_HEAP in assertprop --- src/coreclr/jit/assertionprop.cpp | 159 ++++++++++++++++++------------ src/coreclr/jit/compiler.h | 4 +- 2 files changed, 99 insertions(+), 64 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index ff5387f2c5678..139dbef301c01 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4633,13 +4633,16 @@ GenTree* Compiler::optAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* tr { assert(tree->OperIsIndir()); - bool didNonNullProp = optNonNullAssertionProp_Ind(assertions, tree); - bool didNonHeapProp = optNonHeapAssertionProp_Ind(assertions, tree); - if (didNonNullProp || didNonHeapProp) + bool updated = optNonNullAssertionProp_Ind(assertions, tree); + if (tree->OperIs(GT_STOREIND)) { - return optAssertionProp_Update(tree, tree, stmt); + updated |= optWriteBarrierAssertionProp_StoreInd(assertions, tree->AsStoreInd()); } + if (updated) + { + return optAssertionProp_Update(tree, tree, stmt); + } return nullptr; } @@ -4900,94 +4903,128 @@ bool Compiler::optNonNullAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* } //------------------------------------------------------------------------ -// optAssertionIsNonHeap: see if we can prove a tree's value will be not GC-tracked -// based on assertions +// GetWriteBarrierForm: Determinate the exact type of write barrier required for the +// given address. // // Arguments: -// op - tree to check -// assertions - set of live assertions -// pVnBased - [out] set to true if value numbers were used -// pIndex - [out] the assertion used in the proof +// vnStore - ValueNumStore object +// vn - VN of the address // // Return Value: -// true if the tree's value will be not GC-tracked -// -// Notes: -// Sets "pVnBased" if the assertion is value number based. If no matching -// assertions are found from the table, then returns "NO_ASSERTION_INDEX." +// Exact type of write barrier required for the given address. // -// If both VN and assertion table yield a matching assertion, "pVnBased" -// is only set and the return value is "NO_ASSERTION_INDEX." -// -bool Compiler::optAssertionIsNonHeap(GenTree* op, - ASSERT_VALARG_TP assertions DEBUGARG(bool* pVnBased) - DEBUGARG(AssertionIndex* pIndex)) +static GCInfo::WriteBarrierForm GetWriteBarrierForm(ValueNumStore* vnStore, ValueNum vn) { - bool vnBased = (!optLocalAssertionProp && vnStore->IsVNObjHandle(op->gtVNPair.GetConservative())); -#ifdef DEBUG - *pIndex = NO_ASSERTION_INDEX; - *pVnBased = vnBased; -#endif - - if (vnBased) + const var_types type = vnStore->TypeOfVN(vn); + if (type == TYP_REF) { - return true; + return GCInfo::WriteBarrierForm::WBF_BarrierUnchecked; + } + if (type != TYP_BYREF) + { + return GCInfo::WriteBarrierForm::WBF_BarrierUnknown; } - if (op->IsIconHandle(GTF_ICON_OBJ_HDL)) + VNFuncApp funcApp; + if (vnStore->GetVNFunc(vnStore->VNNormalValue(vn), &funcApp)) { - return true; + if (funcApp.m_func == VNF_PtrToArrElem) + { + // Arrays are always on the heap + return GCInfo::WriteBarrierForm::WBF_BarrierUnchecked; + } + if (funcApp.m_func == VNF_PtrToLoc) + { + // Pointer to a local + return GCInfo::WriteBarrierForm::WBF_NoBarrier; + } + if (funcApp.m_func == VNFunc(GT_ADD)) + { + // Check arguments of the byref add + const GCInfo::WriteBarrierForm arg0Type = GetWriteBarrierForm(vnStore, funcApp.m_args[0]); + if (arg0Type != GCInfo::WriteBarrierForm::WBF_BarrierUnknown) + { + return arg0Type; + } + + const GCInfo::WriteBarrierForm arg1Type = GetWriteBarrierForm(vnStore, funcApp.m_args[1]); + if (arg1Type != GCInfo::WriteBarrierForm::WBF_BarrierUnknown) + { + return arg1Type; + } + } } - return false; + // TODO: + // * addr is ByRefLike - NoBarrier (https://github.com/dotnet/runtime/issues/9512) + // * addr is address of a local - NoBarrier + // + return GCInfo::WriteBarrierForm::WBF_BarrierUnknown; } //------------------------------------------------------------------------ -// optNonHeapAssertionProp_Ind: Possibly prove an indirection not GC-tracked. +// optWriteBarrierAssertionProp_StoreInd: This function assists gcIsWriteBarrierCandidate with help of +// assertions and VNs since CSE may "hide" addresses/values under locals, making it impossible for +// gcIsWriteBarrierCandidate to determine the exact type of write barrier required +// (it's too late for it to rely on VNs). +// +// There are three cases we handle here: +// * Target is not on the heap - no write barrier is required +// * Target could be on the heap, but the value being stored doesn't require any write barrier +// * Target is definitely on the heap - checked (slower) write barrier is not required // // Arguments: // assertions - Active assertions -// indir - The indirection +// indir - The STOREIND node // // Return Value: -// Whether the indirection was found to be not GC-tracked and marked as such. +// Whether the exact type of write barrier was determined and marked on the STOREIND node. // -bool Compiler::optNonHeapAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* indir) +bool Compiler::optWriteBarrierAssertionProp_StoreInd(ASSERT_VALARG_TP assertions, GenTreeStoreInd* indir) { - assert(indir->OperIsIndir()); - - if (!indir->OperIs(GT_STOREIND) || (indir->gtFlags & GTF_IND_TGT_NOT_HEAP) != 0) + if (optLocalAssertionProp || !indir->TypeIs(TYP_REF) || ((indir->gtFlags & GTF_IND_TGT_NOT_HEAP) != 0)) { return false; } -// We like CSE to happen for handles, as the codegen for loading a 64-bit constant can be pretty heavy -// and this is particularly true on platforms with a fixed-width instruction encoding. However, this -// pessimizes stores as we can no longer optimize around some object handles that would allow us to -// bypass the write barrier. -// -// In order to handle that, we'll propagate the IND_TGT_NOT_HEAP flag onto the store if the handle is -// directly or if the underlying value number is an applicable object handle. + const GenTree* value = indir->AsIndir()->Data(); + const GenTree* addr = indir->AsIndir()->Addr(); + const ValueNum valueVN = value->gtVNPair.GetConservative(); + const ValueNum addrVN = addr->gtVNPair.GetConservative(); -#ifdef DEBUG - bool vnBased = false; - AssertionIndex index = NO_ASSERTION_INDEX; -#endif - if (optAssertionIsNonHeap(indir->AsIndir()->Data(), assertions DEBUGARG(&vnBased) DEBUGARG(&index))) + GCInfo::WriteBarrierForm barrierType; + + // First, analyze the value being stored + if (value->IsIntegralConst(0) || valueVN == ValueNumStore::VNForNull()) { -#ifdef DEBUG - if (verbose) - { - (vnBased) ? printf("\nVN based non-heap prop in " FMT_BB ":\n", compCurBB->bbNum) - : printf("\nNon-heap prop for index #%02u in " FMT_BB ":\n", index, compCurBB->bbNum); - gtDispTree(indir, nullptr, nullptr, true); - } -#endif - indir->gtFlags |= GTF_IND_TGT_NOT_HEAP; + // The value being stored is null + barrierType = GCInfo::WriteBarrierForm::WBF_NoBarrier; + } + else if (value->IsIconHandle(GTF_ICON_OBJ_HDL) || vnStore->IsVNObjHandle(valueVN)) + { + // The value being stored is a handle + barrierType = GCInfo::WriteBarrierForm::WBF_NoBarrier; + } + else + { + // Next, analyze the address if we haven't already determined the barrier type from the value + barrierType = GetWriteBarrierForm(vnStore, addrVN); + } + if (barrierType == GCInfo::WriteBarrierForm::WBF_NoBarrier) + { + JITDUMP("Marking STOREIND as not requiring a write barrier:\n"); + DISPTREE(indir); + indir->gtFlags |= GTF_IND_TGT_NOT_HEAP; + return true; + } + if (barrierType == GCInfo::WriteBarrierForm::WBF_BarrierUnchecked) + { + JITDUMP("Marking STOREIND as not requiring a CHECKED write barrier:\n"); + DISPTREE(indir); + indir->gtFlags |= GTF_IND_TGT_HEAP; return true; } - return false; } diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 649de34c7c218..481158aa1d9f0 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7771,8 +7771,6 @@ class Compiler AssertionIndex optAssertionIsNonNullInternal(GenTree* op, ASSERT_VALARG_TP assertions DEBUGARG(bool* pVnBased)); bool optAssertionIsNonNull(GenTree* op, ASSERT_VALARG_TP assertions DEBUGARG(bool* pVnBased) DEBUGARG(AssertionIndex* pIndex)); - bool optAssertionIsNonHeap(GenTree* op, - ASSERT_VALARG_TP assertions DEBUGARG(bool* pVnBased) DEBUGARG(AssertionIndex* pIndex)); AssertionIndex optGlobalAssertionIsEqualOrNotEqual(ASSERT_VALARG_TP assertions, GenTree* op1, GenTree* op2); AssertionIndex optGlobalAssertionIsEqualOrNotEqualZero(ASSERT_VALARG_TP assertions, GenTree* op1); @@ -7809,7 +7807,7 @@ class Compiler GenTree* optAssertionProp_Update(GenTree* newTree, GenTree* tree, Statement* stmt); GenTree* optNonNullAssertionProp_Call(ASSERT_VALARG_TP assertions, GenTreeCall* call); bool optNonNullAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* indir); - bool optNonHeapAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* indir); + bool optWriteBarrierAssertionProp_StoreInd(ASSERT_VALARG_TP assertions, GenTreeStoreInd* indir) const; // Implied assertion functions. void optImpliedAssertions(AssertionIndex assertionIndex, ASSERT_TP& activeAssertions); From 48cdb01f28fef7a0f2aae1bde23ca663eec89cd7 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 5 Feb 2024 01:18:39 +0100 Subject: [PATCH 2/6] clean up --- src/coreclr/jit/assertionprop.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 139dbef301c01..d929a056dec17 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4956,8 +4956,7 @@ static GCInfo::WriteBarrierForm GetWriteBarrierForm(ValueNumStore* vnStore, Valu } // TODO: - // * addr is ByRefLike - NoBarrier (https://github.com/dotnet/runtime/issues/9512) - // * addr is address of a local - NoBarrier + // * addr is ByRefLike - NoBarrier (https://github.com/dotnet/runtime/issues/9512) // return GCInfo::WriteBarrierForm::WBF_BarrierUnknown; } From 77731bdf8e136a683e2be3c250995d358b905798 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 5 Feb 2024 01:27:52 +0100 Subject: [PATCH 3/6] fix compilation --- src/coreclr/jit/compiler.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 481158aa1d9f0..e25c22571c807 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7807,7 +7807,7 @@ class Compiler GenTree* optAssertionProp_Update(GenTree* newTree, GenTree* tree, Statement* stmt); GenTree* optNonNullAssertionProp_Call(ASSERT_VALARG_TP assertions, GenTreeCall* call); bool optNonNullAssertionProp_Ind(ASSERT_VALARG_TP assertions, GenTree* indir); - bool optWriteBarrierAssertionProp_StoreInd(ASSERT_VALARG_TP assertions, GenTreeStoreInd* indir) const; + bool optWriteBarrierAssertionProp_StoreInd(ASSERT_VALARG_TP assertions, GenTreeStoreInd* indir); // Implied assertion functions. void optImpliedAssertions(AssertionIndex assertionIndex, ASSERT_TP& activeAssertions); From a1666d7da8ab6047e7c856de9a94a14551f3f7af Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 5 Feb 2024 03:43:18 +0100 Subject: [PATCH 4/6] Clean up --- src/coreclr/jit/assertionprop.cpp | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index d929a056dec17..1e727ab50632d 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4981,25 +4981,24 @@ static GCInfo::WriteBarrierForm GetWriteBarrierForm(ValueNumStore* vnStore, Valu // bool Compiler::optWriteBarrierAssertionProp_StoreInd(ASSERT_VALARG_TP assertions, GenTreeStoreInd* indir) { - if (optLocalAssertionProp || !indir->TypeIs(TYP_REF) || ((indir->gtFlags & GTF_IND_TGT_NOT_HEAP) != 0)) + const GenTree* value = indir->AsIndir()->Data(); + const GenTree* addr = indir->AsIndir()->Addr(); + + if (optLocalAssertionProp || !indir->TypeIs(TYP_REF) || !value->TypeIs(TYP_REF) || + ((indir->gtFlags & GTF_IND_TGT_NOT_HEAP) != 0)) { return false; } - const GenTree* value = indir->AsIndir()->Data(); - const GenTree* addr = indir->AsIndir()->Addr(); - const ValueNum valueVN = value->gtVNPair.GetConservative(); - const ValueNum addrVN = addr->gtVNPair.GetConservative(); - GCInfo::WriteBarrierForm barrierType; // First, analyze the value being stored - if (value->IsIntegralConst(0) || valueVN == ValueNumStore::VNForNull()) + if (value->IsIntegralConst(0) || (value->gtVNPair.GetConservative() == ValueNumStore::VNForNull())) { // The value being stored is null barrierType = GCInfo::WriteBarrierForm::WBF_NoBarrier; } - else if (value->IsIconHandle(GTF_ICON_OBJ_HDL) || vnStore->IsVNObjHandle(valueVN)) + else if (value->IsIconHandle(GTF_ICON_OBJ_HDL) || vnStore->IsVNObjHandle(value->gtVNPair.GetConservative())) { // The value being stored is a handle barrierType = GCInfo::WriteBarrierForm::WBF_NoBarrier; @@ -5007,7 +5006,7 @@ bool Compiler::optWriteBarrierAssertionProp_StoreInd(ASSERT_VALARG_TP assertions else { // Next, analyze the address if we haven't already determined the barrier type from the value - barrierType = GetWriteBarrierForm(vnStore, addrVN); + barrierType = GetWriteBarrierForm(vnStore, addr->gtVNPair.GetConservative()); } if (barrierType == GCInfo::WriteBarrierForm::WBF_NoBarrier) From e79b58b029fee184d720171205af96b4afe4e0ef Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 5 Feb 2024 12:50:45 +0100 Subject: [PATCH 5/6] Fix TP regression --- src/coreclr/jit/assertionprop.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 1e727ab50632d..e2181b61768a5 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4990,7 +4990,7 @@ bool Compiler::optWriteBarrierAssertionProp_StoreInd(ASSERT_VALARG_TP assertions return false; } - GCInfo::WriteBarrierForm barrierType; + GCInfo::WriteBarrierForm barrierType = GCInfo::WriteBarrierForm::WBF_BarrierUnknown; // First, analyze the value being stored if (value->IsIntegralConst(0) || (value->gtVNPair.GetConservative() == ValueNumStore::VNForNull())) @@ -5003,9 +5003,12 @@ bool Compiler::optWriteBarrierAssertionProp_StoreInd(ASSERT_VALARG_TP assertions // The value being stored is a handle barrierType = GCInfo::WriteBarrierForm::WBF_NoBarrier; } - else + else if ((indir->gtFlags & GTF_IND_TGT_HEAP) == 0) { // Next, analyze the address if we haven't already determined the barrier type from the value + // + // 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()); } From 309f486ef6f3f70449d1b81b221bdbb070d742cb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 8 Feb 2024 02:35:07 +0100 Subject: [PATCH 6/6] Address feedback --- src/coreclr/jit/assertionprop.cpp | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 3be991fd337e5..f1a4c616b0566 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -4906,17 +4906,22 @@ static GCInfo::WriteBarrierForm GetWriteBarrierForm(ValueNumStore* vnStore, Valu } if (funcApp.m_func == VNFunc(GT_ADD)) { - // Check arguments of the byref add - const GCInfo::WriteBarrierForm arg0Type = GetWriteBarrierForm(vnStore, funcApp.m_args[0]); - if (arg0Type != GCInfo::WriteBarrierForm::WBF_BarrierUnknown) + // Check arguments of the GT_ADD + // To make it conservative, we require one of the arguments to be a constant, e.g.: + // + // addressOfLocal + cns -> NoBarrier + // cns + addressWithinHeap -> BarrierUnchecked + // + // Because "addressOfLocal + nativeIntVariable" could be in fact a pointer to the heap. + // if "nativeIntVariable == addressWithinHeap - addressOfLocal". + // + if (vnStore->IsVNConstantNonHandle(funcApp.m_args[0])) { - return arg0Type; + return GetWriteBarrierForm(vnStore, funcApp.m_args[1]); } - - const GCInfo::WriteBarrierForm arg1Type = GetWriteBarrierForm(vnStore, funcApp.m_args[1]); - if (arg1Type != GCInfo::WriteBarrierForm::WBF_BarrierUnknown) + if (vnStore->IsVNConstantNonHandle(funcApp.m_args[1])) { - return arg1Type; + return GetWriteBarrierForm(vnStore, funcApp.m_args[0]); } } } @@ -4978,20 +4983,21 @@ bool Compiler::optWriteBarrierAssertionProp_StoreInd(ASSERT_VALARG_TP assertions barrierType = GetWriteBarrierForm(vnStore, addr->gtVNPair.GetConservative()); } + JITDUMP("Trying to determine the exact type of write barrier for STOREIND [%d06]: ", dspTreeID(indir)); if (barrierType == GCInfo::WriteBarrierForm::WBF_NoBarrier) { - JITDUMP("Marking STOREIND as not requiring a write barrier:\n"); - DISPTREE(indir); + JITDUMP("is not needed at all.\n"); indir->gtFlags |= GTF_IND_TGT_NOT_HEAP; return true; } if (barrierType == GCInfo::WriteBarrierForm::WBF_BarrierUnchecked) { - JITDUMP("Marking STOREIND as not requiring a CHECKED write barrier:\n"); - DISPTREE(indir); + JITDUMP("unchecked is fine.\n"); indir->gtFlags |= GTF_IND_TGT_HEAP; return true; } + + JITDUMP("unknown (checked).\n"); return false; }