From 0ef3ab76370a5691163ad521ebfc4e9d27f11ea9 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 60ca01ba9f0aad8a64b441ff476f7b61751a1521 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 7fe9bffa70e943..c426cf37e2e1a0 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 7289665b79f8efcce4018f446fbaa5dc05c33e7d 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; } - }