From 9737d079a164f2920ef60d878d9b7e0bb5d66dcd Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 24 Aug 2023 15:21:14 +0200 Subject: [PATCH] JIT: Fix physical promotion creating overlapping local uses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Physical promotion could in some cases create uses overlapping illegally with defs when faced with seemingly last uses of structs. This is a result of a mismatch between our model for liveness and the actual model of local uses in the backend. In the actual model, the uses of LCL_VARs occur at the user, which means that it is possible for there to be no place at which to insert IR between to local uses. The example looks like the following. Physical promotion would be faced with a tree like ``` ▌ CALL void Program:Foo(Program+S,Program+S) ├──▌ LCL_VAR struct V01 loc0 └──▌ LCL_VAR struct V01 loc0 ``` When V01 was fully promoted, both of these are logically last uses since all state of V01 is stored in promoted field locals. Because of that we would make the following transformation: ``` ▌ CALL void Program:Foo(Program+S,Program+S) ├──▌ LCL_VAR struct V01 loc0 (last use) └──▌ COMMA struct ├──▌ STORE_LCL_FLD int V01 loc0 [+0] │ └──▌ LCL_VAR int V02 tmp0 └──▌ LCL_VAR struct V01 loc0 (last use) ``` This creates an illegally overlapping use and def; additionally, it is correct only in a world where the store actually would happen between the two uses. It is also moderately dangerous to mark both of these as last uses given the implicit byref transformation. The fix is to avoid marking a struct use as a last use if we see more struct uses in the same statement. Fix #91056 --- src/coreclr/jit/promotion.cpp | 43 +++++++++++++++++-- .../Runtime_81725/Runtime_81725.csproj | 2 - .../Runtime_85088/Runtime_85088.csproj | 2 + .../JitBlue/Runtime_91056/Runtime_91056.cs | 33 ++++++++++++++ .../Runtime_91056/Runtime_91056.csproj | 12 ++++++ 5 files changed, 87 insertions(+), 5 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_91056/Runtime_91056.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_91056/Runtime_91056.csproj diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index 5982ed7928335..52163f4db0cce 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -2340,8 +2340,36 @@ void ReplaceVisitor::ReadBackAfterCall(GenTreeCall* call, GenTree* user) // // If the remainder of the struct local is dying, then we expect that this // entire struct local is now dying, since all field accesses are going to be -// replaced with other locals. The exception is if there is a queued read -// back for any of the fields. +// replaced with other locals. +// +// There are two exceptions to the above: +// +// 1) If there is a queued readback for any of the fields, then there is +// live state in the struct local, so it is not dying. +// +// 2) If there are further uses of the local in the same statement then we cannot +// actually act on the last-use information we would provide here. That's because +// uses of locals occur at the user and we do not model that here. In the real model +// there are cases where we do not have any place to insert any IR between the two uses. +// For example, consider: +// +// ▌ CALL void Program:Foo(Program+S,Program+S) +// ├──▌ LCL_VAR struct V01 loc0 +// └──▌ LCL_VAR struct V01 loc0 +// +// If V01 is promoted fully then both uses of V01 are last uses here; but +// replacing the IR with +// +// ▌ CALL void Program:Foo(Program+S,Program+S) +// ├──▌ LCL_VAR struct V01 loc0 (last use) +// └──▌ COMMA struct +// ├──▌ STORE_LCL_FLD int V01 loc0 [+0] +// │ └──▌ LCL_VAR int V02 tmp0 +// └──▌ LCL_VAR struct V01 loc0 (last use) +// +// would be illegal since the created store overlaps with the first local, +// and does not take into account that both uses occur simultaneously at +// the position of the CALL node. // bool ReplaceVisitor::IsPromotedStructLocalDying(GenTreeLclVarCommon* lcl) { @@ -2362,6 +2390,15 @@ bool ReplaceVisitor::IsPromotedStructLocalDying(GenTreeLclVarCommon* lcl) } } + for (GenTree* cur = lcl->gtNext; cur != nullptr; cur = cur->gtNext) + { + assert(cur->OperIsAnyLocal()); + if (cur->TypeIs(TYP_STRUCT) && (cur->AsLclVarCommon()->GetLclNum() == lcl->GetLclNum())) + { + return false; + } + } + return true; } @@ -2577,7 +2614,7 @@ void ReplaceVisitor::WriteBackBeforeCurrentStatement(unsigned lcl, unsigned offs GenTree* readBack = Promotion::CreateWriteBack(m_compiler, lcl, rep); Statement* stmt = m_compiler->fgNewStmtFromTree(readBack); - JITDUMP("Writing back %s before " FMT_STMT "\n", rep.Description, stmt->GetID()); + JITDUMP("Writing back %s before " FMT_STMT "\n", rep.Description, m_currentStmt->GetID()); DISPSTMT(stmt); m_compiler->fgInsertStmtBefore(m_currentBlock, m_currentStmt, stmt); ClearNeedsWriteBack(rep); diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_81725/Runtime_81725.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_81725/Runtime_81725.csproj index b9493ef3d767c..8d9ddba76b933 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_81725/Runtime_81725.csproj +++ b/src/tests/JIT/Regression/JitBlue/Runtime_81725/Runtime_81725.csproj @@ -7,8 +7,6 @@ - - diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.csproj index 85f04c1ebc8f7..d0c820dcec6ea 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.csproj +++ b/src/tests/JIT/Regression/JitBlue/Runtime_85088/Runtime_85088.csproj @@ -1,5 +1,7 @@ + + true True diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_91056/Runtime_91056.cs b/src/tests/JIT/Regression/JitBlue/Runtime_91056/Runtime_91056.cs new file mode 100644 index 0000000000000..a0c78804a7ee5 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_91056/Runtime_91056.cs @@ -0,0 +1,33 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Xunit; +using System.Runtime.CompilerServices; + +public class Runtime_91056 +{ + [Fact] + public static void TestEntryPoint() + { + S s = default; + if (False()) + { + s.A = 1234; + } + + Foo(0, 0, s, s); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool False() => false; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Foo(int a, int b, S s1, S s2) + { + } + + public struct S + { + public int A; + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_91056/Runtime_91056.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_91056/Runtime_91056.csproj new file mode 100644 index 0000000000000..444d119c04fe9 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_91056/Runtime_91056.csproj @@ -0,0 +1,12 @@ + + + + true + True + + + + + + + \ No newline at end of file