Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: Fix block morphing/physical promotion self-interference #85887

Merged
merged 3 commits into from
May 9, 2023
Merged

JIT: Fix block morphing/physical promotion self-interference #85887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8991220
JIT: Fix block morphing/physical promotion self-interference
jakobbotsch May 7, 2023
36475ea
Update src/coreclr/jit/morphblock.cpp
jakobbotsch May 7, 2023
3563c9f
Update src/coreclr/jit/promotion.h
jakobbotsch May 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 62 additions & 2 deletions src/coreclr/jit/morphblock.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,9 @@ class MorphCopyBlockHelper : public MorphInitBlockHelper

bool m_dstDoFldAsg = false;
bool m_srcDoFldAsg = false;

private:
bool CanReuseAddressForDecomposedStore(GenTree* addr);
};

//------------------------------------------------------------------------
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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<Program+ListElement, 16>(P) V00 loc0
// ▌ long V00.Program+ListElement:Next (offs=0x00) -> V03 tmp1
// ▌ int V00.Program+ListElement:Value (offs=0x08) -> V04 tmp2
// └──▌ BLK struct<Program+ListElement, 16>
// └──▌ 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.
//
Expand Down
69 changes: 59 additions & 10 deletions src/coreclr/jit/promotiondecomposition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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.
Expand Down
47 changes: 47 additions & 0 deletions src/tests/JIT/Directed/physicalpromotion/addressinterference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>(T val)
{
}

struct ListElement
{
public ListElement* Next;
public int Value;
}

}
10 changes: 10 additions & 0 deletions src/tests/JIT/Directed/physicalpromotion/addressinterference.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
<AllowUnsafeBlocks>True</AllowUnsafeBlocks>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
<CLRTestEnvironmentVariable Include="DOTNET_JitStressModeNames" Value="STRESS_PHYSICAL_PROMOTION STRESS_PHYSICAL_PROMOTION_COST STRESS_NO_OLD_PROMOTION" />
</ItemGroup>
</Project>
47 changes: 47 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_85874/Runtime_85874.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised we haven't stumbled on this one before.

Consume(e1);
return e1.Value;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static void Consume<T>(T val)
{
}

struct ListElement
{
public ListElement* Next;
public int Value;
}

}
9 changes: 9 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_85874/Runtime_85874.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
<AllowUnsafeBlocks>True</AllowUnsafeBlocks>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>