diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index 7fe9bffa70e94..c426cf37e2e1a 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -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(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(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. diff --git a/src/coreclr/jit/promotion.h b/src/coreclr/jit/promotion.h index 28481a97eaf88..a8c54d4000f34 100644 --- a/src/coreclr/jit/promotion.h +++ b/src/coreclr/jit/promotion.h @@ -290,6 +290,7 @@ class ReplaceVisitor : public GenTreeVisitor bool VisitOverlappingReplacements(unsigned lcl, unsigned offs, unsigned size, Func func); void InsertPreStatementReadBacks(); + void InsertPreStatementReadBackIfNecessary(unsigned aggLclNum, Replacement& rep); void InsertPreStatementWriteBacks(); GenTree** InsertMidTreeReadBacks(GenTree** use); diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs b/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs new file mode 100644 index 0000000000000..df3f9af0ee54c --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs @@ -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 x) + { + } + + private struct S + { + public int A, B, C, D; + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.csproj new file mode 100644 index 0000000000000..de6d5e08882e8 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.csproj @@ -0,0 +1,8 @@ + + + True + + + + +