From ffb3523fa5bbc46641ef4896fa9c7c1369600435 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 18 Sep 2026 07:55:19 -0700 Subject: [PATCH] JIT: Fix promoted liveness in forward substitution (#133703) Multi-use forward substitution could leave stale last-use flags on promoted struct parents, allowing a live struct copy to be removed. Make last-use invalidation promotion-aware and reuse the same logic for single- and multi-use substitution. Add a regression test. Fixes #133641 > [!NOTE] > This PR description was generated with GitHub Copilot. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/forwardsub.cpp | 93 ++----------------- .../JIT/Regression_ro_2/Runtime_133641.cs | 33 +++++++ 2 files changed, 41 insertions(+), 85 deletions(-) create mode 100644 src/tests/JIT/Regression_ro_2/Runtime_133641.cs diff --git a/src/coreclr/jit/forwardsub.cpp b/src/coreclr/jit/forwardsub.cpp index 4564d8dc3e4d50..40077ab501172d 100644 --- a/src/coreclr/jit/forwardsub.cpp +++ b/src/coreclr/jit/forwardsub.cpp @@ -222,97 +222,20 @@ bool Compiler::fgForwardSubMultiUse(Statement* nextStmt, unsigned lclNum, GenTre return false; } - // Pre-allocate every clone up front so we can bail without mutating the IR if - // gtCloneExpr ever refuses to duplicate the tree. - int const lastIdx = useCount - 1; - ArrayStack clones(getAllocator(CMK_Generic)); + int const lastIdx = useCount - 1; for (int i = 0; i < lastIdx; i++) { - GenTree* const clone = gtCloneExpr(fwdSubNode); - if (clone == nullptr) - { - return false; - } - clones.Push(clone); - } - - // Replace all-but-last use sites with a clone; the last use site gets the original tree. - for (int i = 0; i < lastIdx; i++) - { - *v.m_useSlots.BottomRef(i) = clones.Bottom(i); + *v.m_useSlots.BottomRef(i) = gtCloneExpr(fwdSubNode); } *v.m_useSlots.BottomRef(lastIdx) = fwdSubNode; - // After substitution we have N clones inserted at the original use sites - // of `lclNum`, each potentially referencing locals that already appeared - // elsewhere in `nextStmt`. Two correctness fixups are required: - // - // (a) GTF_VAR_DEATH_MASK was copied from the def position by gtCloneExpr; - // any one (or all) copies may have death bits that are no longer - // semantically valid now that there are multiple copies. - // (b) Earlier LCL_VAR references in nextStmt to one of the locals appearing - // inside fwdSubNode may have been a "last use" of that local; the new - // copies make them no longer last. - // - // Be conservative: clear GTF_VAR_DEATH_MASK on every LCL_VAR in nextStmt - // whose lclNum appears anywhere in fwdSubNode. This is the multi-use analogue - // of fgForwardSubUpdateLiveness. - struct CollectLclNumsVisitor : public GenTreeVisitor - { - enum - { - DoPreOrder = true, - }; - - ArrayStack m_lclNums; - - CollectLclNumsVisitor(Compiler* comp) - : GenTreeVisitor(comp) - , m_lclNums(comp->getAllocator(CMK_Generic)) - { - } - - fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) - { - GenTree* node = *use; - if (node->OperIsLocal()) - { - unsigned const ln = node->AsLclVarCommon()->GetLclNum(); - bool seen = false; - for (int i = 0; i < m_lclNums.Height(); i++) - { - if (m_lclNums.Bottom(i) == ln) - { - seen = true; - break; - } - } - if (!seen) - { - m_lclNums.Push(ln); - } - } - return fgWalkResult::WALK_CONTINUE; - } - }; - - CollectLclNumsVisitor cnv(this); - cnv.WalkTree(&fwdSubNode, nullptr); - + GenTreeLclVarCommon* const lastUseLcl = gtPeelFieldAddrs(fwdSubNode)->AsLclVarCommon(); fgSequenceLocals(nextStmt); - for (GenTreeLclVarCommon* lcl : nextStmt->LocalsTreeList()) - { - unsigned const ln = lcl->GetLclNum(); - for (int i = 0; i < cnv.m_lclNums.Height(); i++) - { - if (cnv.m_lclNums.Bottom(i) == ln) - { - lcl->gtFlags &= ~GTF_VAR_DEATH_MASK; - break; - } - } - } + // The inserted subtree has exactly one local node, which serves as both the + // start and end of the inserted locals segment. This call walks backward + // from this point, properly adjusting any earlier clone and promoted parent flags. + fgForwardSubUpdateLiveness(lastUseLcl, lastUseLcl); gtUpdateStmtSideEffects(nextStmt); return true; @@ -1112,7 +1035,7 @@ bool Compiler::fgForwardSubStatement(Statement* stmt) { if (!fgForwardSubMultiUse(nextStmt, lclNum, fwdSubNode)) { - JITDUMP(" multi-use sub failed (count out of range, indirect-call context, or clone failed)\n"); + JITDUMP(" multi-use sub failed (count out of range or indirect-call context)\n"); return false; } diff --git a/src/tests/JIT/Regression_ro_2/Runtime_133641.cs b/src/tests/JIT/Regression_ro_2/Runtime_133641.cs new file mode 100644 index 00000000000000..30d27b9b510af6 --- /dev/null +++ b/src/tests/JIT/Regression_ro_2/Runtime_133641.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 System.Runtime.CompilerServices; +using Xunit; + +public class Runtime_133641 +{ + private struct S + { + public long A; + public long B; + public long C; + } + + [Fact] + public static void TestEntryPoint() + { + Assert.Equal(8L, Test(new S { A = 1, B = 2, C = 3 })); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static long Combine(S s, long a, long b) => s.A + s.B + s.C + a + b; + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static long Test(S input) + { + S s = input; + input.A = 42; + long p = s.A; + return Combine(s, p, p); + } +}