diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index 67c2029c03dcaf..f72ed242f5bdb9 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -662,6 +662,9 @@ class MorphCopyBlockHelper : public MorphInitBlockHelper bool m_dstDoFldAsg = false; bool m_srcDoFldAsg = false; + +private: + bool CanReuseAddressForDecomposedStore(GenTree* addr); }; //------------------------------------------------------------------------ @@ -1170,7 +1173,7 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() // and spill, unless we only end up using the address once. if (fieldCnt - dyingFieldCnt > 1) { - if (m_comp->gtClone(srcAddr)) + if (CanReuseAddressForDecomposedStore(srcAddr)) { // "srcAddr" is simple expression. No need to spill. noway_assert((srcAddr->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) == 0); @@ -1202,7 +1205,7 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() // and spill, unless we only end up using the address once. if (m_srcVarDsc->lvFieldCnt > 1) { - if (m_comp->gtClone(dstAddr)) + if (CanReuseAddressForDecomposedStore(dstAddr)) { // "dstAddr" is simple expression. No need to spill noway_assert((dstAddr->gtFlags & GTF_PERSISTENT_SIDE_EFFECTS) == 0); @@ -1512,6 +1515,63 @@ GenTree* MorphCopyBlockHelper::CopyFieldByField() return result; } +//------------------------------------------------------------------------ +// CanReuseAddressForDecomposedStore: Check if it is safe to reuse the +// specified address node for each decomposed store of a block copy. +// +// Arguments: +// addrNode - The address node +// +// Return Value: +// True if the caller can reuse the address by cloning. +// +bool MorphCopyBlockHelper::CanReuseAddressForDecomposedStore(GenTree* addrNode) +{ + if (addrNode->OperIsLocalRead()) + { + GenTreeLclVarCommon* lcl = addrNode->AsLclVarCommon(); + unsigned lclNum = lcl->GetLclNum(); + LclVarDsc* lclDsc = m_comp->lvaGetDesc(lclNum); + if (lclDsc->IsAddressExposed()) + { + // Address could be pointing to itself + return false; + } + + // The store can also directly write to the address. For example: + // + // ▌ STORE_LCL_VAR struct(P) V00 loc0 + // ▌ long V00.Program+ListElement:Next (offs=0x00) -> V03 tmp1 + // ▌ int V00.Program+ListElement:Value (offs=0x08) -> V04 tmp2 + // └──▌ BLK struct + // └──▌ LCL_VAR long V03 tmp1 (last use) + // + // If we reused the address we would produce + // + // ▌ COMMA void + // ├──▌ STORE_LCL_VAR long V03 tmp1 + // │ └──▌ IND long + // │ └──▌ LCL_VAR long V03 tmp1 (last use) + // └──▌ STORE_LCL_VAR int V04 tmp2 + // └──▌ IND int + // └──▌ ADD byref + // ├──▌ LCL_VAR long V03 tmp1 (last use) + // └──▌ CNS_INT long 8 + // + // which is also obviously incorrect. + // + + if (m_dstLclNum == BAD_VAR_NUM) + { + return true; + } + + return (lclNum != m_dstLclNum) && (!lclDsc->lvIsStructField || (lclDsc->lvParentLcl != m_dstLclNum)); + } + + return addrNode->IsInvariant(); +} + //------------------------------------------------------------------------ // fgMorphCopyBlock: Perform the morphing of a block copy. // diff --git a/src/coreclr/jit/promotiondecomposition.cpp b/src/coreclr/jit/promotiondecomposition.cpp index 076e3f720d67a9..1877e85f954262 100644 --- a/src/coreclr/jit/promotiondecomposition.cpp +++ b/src/coreclr/jit/promotiondecomposition.cpp @@ -814,7 +814,6 @@ class DecompositionPlan for (int i = 0; i < m_entries.Height(); i++) { const Entry& entry = m_entries.BottomRef(i); - // TODO: Double check that TYP_BYREF do not incur any write barriers. if ((entry.FromReplacement != nullptr) && (entry.Type == TYP_REF)) { Replacement* rep = entry.FromReplacement; @@ -901,16 +900,14 @@ class DecompositionPlan if ((addr != nullptr) && (numAddrUses > 1)) { - if (addr->OperIsLocal() && (!m_dst->OperIs(GT_LCL_VAR, GT_LCL_FLD) || - (addr->AsLclVarCommon()->GetLclNum() != m_dst->AsLclVarCommon()->GetLclNum()))) + if (CanReuseAddressForDecomposedStore(addr)) { - // We will introduce more uses of the address local, so it is - // no longer dying here. - addr->gtFlags &= ~GTF_VAR_DEATH; - } - else if (addr->IsInvariant()) - { - // Fall through + if (addr->OperIsLocalRead()) + { + // We will introduce more uses of the address local, so it is + // no longer dying here. + addr->gtFlags &= ~GTF_VAR_DEATH; + } } else { @@ -1114,6 +1111,58 @@ class DecompositionPlan !entry.FromReplacement->NeedsWriteBack && (entry.ToLclNum == BAD_VAR_NUM); } + //------------------------------------------------------------------------ + // CanReuseAddressForDecomposedStore: Check if it is safe to reuse the + // specified address node for each decomposed store of a block copy. + // + // Arguments: + // addrNode - The address node + // + // Return Value: + // True if the caller can reuse the address by cloning. + // + bool CanReuseAddressForDecomposedStore(GenTree* addrNode) + { + if (addrNode->OperIsLocalRead()) + { + GenTreeLclVarCommon* lcl = addrNode->AsLclVarCommon(); + unsigned lclNum = lcl->GetLclNum(); + if (m_compiler->lvaGetDesc(lclNum)->IsAddressExposed()) + { + // Address could be pointing to itself + return false; + } + + // If we aren't writing a local here then since the address is not + // exposed it cannot change. + if (!m_dst->OperIs(GT_LCL_VAR, GT_LCL_FLD)) + { + return true; + } + + // Otherwise it could still be possible that the address is part of + // the struct we're writing. + unsigned dstLclNum = m_dst->AsLclVarCommon()->GetLclNum(); + if (lclNum == dstLclNum) + { + return false; + } + + // It could also be one of the replacement locals we're going to write. + for (int i = 0; i < m_entries.Height(); i++) + { + if (m_entries.BottomRef(i).ToLclNum == lclNum) + { + return false; + } + } + + return true; + } + + return addrNode->IsInvariant(); + } + //------------------------------------------------------------------------ // PropagateIndirFlags: // Propagate the specified flags to a GT_IND node. diff --git a/src/tests/JIT/Directed/physicalpromotion/addressinterference.cs b/src/tests/JIT/Directed/physicalpromotion/addressinterference.cs new file mode 100644 index 00000000000000..ac140bc696afcd --- /dev/null +++ b/src/tests/JIT/Directed/physicalpromotion/addressinterference.cs @@ -0,0 +1,47 @@ +// 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; +using Xunit; + +public unsafe class PhysicalPromotionAddressInterference +{ + [Fact] + public static int Exposed() + { + ListElement e1; + e1.Next = &e1; + ListElement e2; + e2.Next = null; + e2.Value = 100; + *e1.Next = e2; + return e1.Value; + } + + [Fact] + public static int DestinationIsAddress() + { + ListElement e1; + ListElement e2 = default; + e2.Value = 100; + e1.Next = &e2; + e1.Value = 1234; + Consume(e1); + e1 = *e1.Next; + Consume(e1); + return e1.Value; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Consume(T val) + { + } + + struct ListElement + { + public ListElement* Next; + public int Value; + } + +} diff --git a/src/tests/JIT/Directed/physicalpromotion/addressinterference.csproj b/src/tests/JIT/Directed/physicalpromotion/addressinterference.csproj new file mode 100644 index 00000000000000..865038c7d08976 --- /dev/null +++ b/src/tests/JIT/Directed/physicalpromotion/addressinterference.csproj @@ -0,0 +1,10 @@ + + + True + True + + + + + + diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_85874/Runtime_85874.cs b/src/tests/JIT/Regression/JitBlue/Runtime_85874/Runtime_85874.cs new file mode 100644 index 00000000000000..3091678200b277 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_85874/Runtime_85874.cs @@ -0,0 +1,47 @@ +// 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; +using Xunit; + +public unsafe class Runtime_85874 +{ + [Fact] + public static int Exposed() + { + ListElement e1; + e1.Next = &e1; + ListElement e2; + e2.Next = null; + e2.Value = 100; + *e1.Next = e2; + return e1.Value; + } + + [Fact] + public static int DestinationIsAddress() + { + ListElement e1; + ListElement e2 = default; + e2.Value = 100; + e1.Next = &e2; + e1.Value = 1234; + Consume(e1); + e1 = *e1.Next; + Consume(e1); + return e1.Value; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Consume(T val) + { + } + + struct ListElement + { + public ListElement* Next; + public int Value; + } + +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_85874/Runtime_85874.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_85874/Runtime_85874.csproj new file mode 100644 index 00000000000000..a4cc9d0594f93e --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_85874/Runtime_85874.csproj @@ -0,0 +1,9 @@ + + + True + True + + + + +