From cc9874b8f37ccd3b787a6cea50983530e9ac1a21 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 22 Oct 2024 12:30:39 +0200 Subject: [PATCH 1/3] Add a test --- .../JitBlue/Runtime_108969/Runtime_108969.cs | 56 +++++++++++++++++++ .../Runtime_108969/Runtime_108969.csproj | 8 +++ 2 files changed, 64 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.csproj 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 00000000000000..7ca5b0db7ea793 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs @@ -0,0 +1,56 @@ +// 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 00000000000000..de6d5e08882e86 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.csproj @@ -0,0 +1,8 @@ + + + True + + + + + From e41aff5ce8db2fa7de12336107bad3000419bd7c Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 22 Oct 2024 12:46:38 +0200 Subject: [PATCH 2/3] JIT: Read back all replacements before statements with implicit EH control flow 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 --- src/coreclr/jit/promotion.cpp | 86 +++++++++++++++++++++++++---------- src/coreclr/jit/promotion.h | 1 + 2 files changed, 63 insertions(+), 24 deletions(-) diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index d3051286c34c2a..a1baf4cc3768ec 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 28481a97eaf88b..a8c54d4000f341 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); From e07c035b2031e5a95ff10647b7d54ab2385b5a1c Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 22 Oct 2024 12:54:29 +0200 Subject: [PATCH 3/3] Remove space --- .../JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs b/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs index 7ca5b0db7ea793..df3f9af0ee54ce 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_108969/Runtime_108969.cs @@ -52,5 +52,4 @@ private struct S { public int A, B, C, D; } - }