Skip to content

Commit

Permalink
[release/9.0-staging] JIT: Read back all replacements before statemen…
Browse files Browse the repository at this point in the history
…ts with implicit EH control flow (#109143)

Physical promotion sometimes needs to insert read backs of all stale
pending replacements when it encounters potential implicit EH control
flow (that is, intra-function control flow because of locally caught
exceptions). It would be possible for this to interact with QMARKs such
that we inserted readbacks inside one of the branches, yet believed we
had read back the replacement on all paths.

The fix here is to indiscriminately start reading back all replacements
before a statement that may cause potential intra-function control flow
to occur.

Fix #108969
  • Loading branch information
github-actions[bot] authored Dec 13, 2024
1 parent 30c4237 commit fc3708f
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 24 deletions.
86 changes: 62 additions & 24 deletions src/coreclr/jit/promotion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1885,41 +1885,79 @@ void ReplaceVisitor::InsertPreStatementReadBacks()
// 3. Creating embedded stores in ReplaceLocal disables local copy prop for
// that local (see ReplaceLocal).

for (GenTreeLclVarCommon* lcl : m_currentStmt->LocalsTreeList())
// Normally, we read back only for the uses we will see. However, with
// implicit EH flow we may also read back all replacements mid-tree (see
// InsertMidTreeReadBacks). So for that case we read back everything. This
// is a correctness requirement for QMARKs, but we do it indiscriminately
// for the same reasons as mentioned above.
if (((m_currentStmt->GetRootNode()->gtFlags & (GTF_EXCEPT | GTF_CALL)) != 0) &&
m_compiler->ehBlockHasExnFlowDsc(m_currentBlock))
{
if (lcl->TypeIs(TYP_STRUCT))
{
continue;
}
JITDUMP(
"Reading back pending replacements before statement with possible exception side effect inside block in try region\n");

AggregateInfo* agg = m_aggregates.Lookup(lcl->GetLclNum());
if (agg == nullptr)
for (AggregateInfo* agg : m_aggregates)
{
continue;
for (Replacement& rep : agg->Replacements)
{
InsertPreStatementReadBackIfNecessary(agg->LclNum, rep);
}
}

size_t index = Promotion::BinarySearch<Replacement, &Replacement::Offset>(agg->Replacements, lcl->GetLclOffs());
if ((ssize_t)index < 0)
}
else
{
// Otherwise just read back the locals we see uses of.
for (GenTreeLclVarCommon* lcl : m_currentStmt->LocalsTreeList())
{
continue;
}
if (lcl->TypeIs(TYP_STRUCT))
{
continue;
}

Replacement& rep = agg->Replacements[index];
if (rep.NeedsReadBack)
{
JITDUMP("Reading back replacement V%02u.[%03u..%03u) -> V%02u before [%06u]:\n", agg->LclNum, rep.Offset,
rep.Offset + genTypeSize(rep.AccessType), rep.LclNum,
Compiler::dspTreeID(m_currentStmt->GetRootNode()));
AggregateInfo* agg = m_aggregates.Lookup(lcl->GetLclNum());
if (agg == nullptr)
{
continue;
}

GenTree* readBack = Promotion::CreateReadBack(m_compiler, agg->LclNum, rep);
Statement* stmt = m_compiler->fgNewStmtFromTree(readBack);
DISPSTMT(stmt);
m_compiler->fgInsertStmtBefore(m_currentBlock, m_currentStmt, stmt);
ClearNeedsReadBack(rep);
size_t index =
Promotion::BinarySearch<Replacement, &Replacement::Offset>(agg->Replacements, lcl->GetLclOffs());
if ((ssize_t)index < 0)
{
continue;
}

InsertPreStatementReadBackIfNecessary(agg->LclNum, agg->Replacements[index]);
}
}
}

//------------------------------------------------------------------------
// InsertPreStatementReadBackIfNecessary:
// Insert a read back of the specified replacement before the current
// statement, if the replacement needs it.
//
// Parameters:
// aggLclNum - Struct local
// rep - The replacement
//
void ReplaceVisitor::InsertPreStatementReadBackIfNecessary(unsigned aggLclNum, Replacement& rep)
{
if (!rep.NeedsReadBack)
{
return;
}

JITDUMP("Reading back replacement V%02u.[%03u..%03u) -> V%02u before [%06u]:\n", aggLclNum, rep.Offset,
rep.Offset + genTypeSize(rep.AccessType), rep.LclNum, Compiler::dspTreeID(m_currentStmt->GetRootNode()));

GenTree* readBack = Promotion::CreateReadBack(m_compiler, aggLclNum, rep);
Statement* stmt = m_compiler->fgNewStmtFromTree(readBack);
DISPSTMT(stmt);
m_compiler->fgInsertStmtBefore(m_currentBlock, m_currentStmt, stmt);
ClearNeedsReadBack(rep);
}

//------------------------------------------------------------------------
// VisitOverlappingReplacements:
// Call a function for every replacement that overlaps a specified segment.
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/promotion.h
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,7 @@ class ReplaceVisitor : public GenTreeVisitor<ReplaceVisitor>
bool VisitOverlappingReplacements(unsigned lcl, unsigned offs, unsigned size, Func func);

void InsertPreStatementReadBacks();
void InsertPreStatementReadBackIfNecessary(unsigned aggLclNum, Replacement& rep);
void InsertPreStatementWriteBacks();
GenTree** InsertMidTreeReadBacks(GenTree** use);

Expand Down
55 changes: 55 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// 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 System.Runtime.Intrinsics;
using Xunit;

public class Runtime_108969
{
[Fact]
public static int TestEntryPoint() => Foo(null);

[MethodImpl(MethodImplOptions.NoInlining)]
private static int Foo(object o)
{
S v = default;
try
{
v = Bar();

// "(int?)o" creates a QMARK with a branch that may throw; we would
// end up reading back v.A inside the QMARK
Use((int?)o);
}
catch (Exception)
{
}

// Induce promotion of v.A field
Use(v.A);
Use(v.A);
Use(v.A);
Use(v.A);
Use(v.A);
Use(v.A);
return v.A;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static S Bar()
{
return new S { A = 100 };
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static void Use<T>(T x)
{
}

private struct S
{
public int A, B, C, D;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>

0 comments on commit fc3708f

Please sign in to comment.