From aaa1de18a7f97774fd44a1728dafc0d86894800e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 9 May 2023 09:27:03 +0200 Subject: [PATCH] JIT: Fix block morphing/physical promotion self-interference (#85887) Fix a couple of cases where it's possible that the decomposed stores will modify the address being used as part of the operation too early. In those cases we must spill the address to a new local ahead of time. Fix #85874 --- src/coreclr/jit/morphblock.cpp | 64 ++++++++++++++++- src/coreclr/jit/promotiondecomposition.cpp | 69 ++++++++++++++++--- .../physicalpromotion/addressinterference.cs | 47 +++++++++++++ .../addressinterference.csproj | 10 +++ .../JitBlue/Runtime_85874/Runtime_85874.cs | 47 +++++++++++++ .../Runtime_85874/Runtime_85874.csproj | 9 +++ 6 files changed, 234 insertions(+), 12 deletions(-) create mode 100644 src/tests/JIT/Directed/physicalpromotion/addressinterference.cs create mode 100644 src/tests/JIT/Directed/physicalpromotion/addressinterference.csproj create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_85874/Runtime_85874.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_85874/Runtime_85874.csproj diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index 67c2029c03dca..f72ed242f5bdb 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 c1f4e6131aa9f..4239694a8bcae 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 { @@ -1104,6 +1101,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 0000000000000..ac140bc696afc --- /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 0000000000000..865038c7d0897 --- /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 0000000000000..3091678200b27 --- /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 0000000000000..a4cc9d0594f93 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_85874/Runtime_85874.csproj @@ -0,0 +1,9 @@ + + + True + True + + + + +