From d7c239db033400c6e2e22578b3da0375f36aebcd Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 23 Apr 2025 10:55:37 -0700 Subject: [PATCH 1/3] JIT: fix issues in field-wise escape analysis Two fixes: * we can't retype gc struct params, as their GC info is reported by the caller. Instead we must mark them as escaping. * when retyping GT_STOREIND we should always use the stored data's new type Fixes #111922. --- src/coreclr/jit/objectalloc.cpp | 18 +++- .../ObjectStackAllocation/Runtime_111922.cs | 86 +++++++++++++++++++ .../Runtime_111922.csproj | 9 ++ 3 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.cs create mode 100644 src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.csproj diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index c45af95d17e86a..eba1604336ff0f 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -698,11 +698,24 @@ void ObjectAllocator::MarkEscapingVarsAndBuildConnGraph() { JITDUMP(" V%02u is address exposed\n", lclNum); MarkLclVarAsEscaping(lclNum); + continue; } - else if (lclNum == comp->info.compRetBuffArg) + + if (lclNum == comp->info.compRetBuffArg) { JITDUMP(" V%02u is retbuff\n", lclNum); MarkLclVarAsEscaping(lclNum); + continue; + } + + // We have to mark all struct params as escaping, because + // their GC reporting is controlled by the caller + // + if (lclDsc->lvIsParam && (lclDsc->lvType == TYP_STRUCT)) + { + JITDUMP(" V%02u is a struct param\n", lclNum); + MarkLclVarAsEscaping(lclNum); + continue; } // Parameters have unknown initial values. @@ -2047,8 +2060,7 @@ void ObjectAllocator::UpdateAncestorTypes(GenTree* tree, // If we are storing to a GC struct field, we may need to retype the store // - if (retypeFields && parent->OperIs(GT_STOREIND) && (addr->OperIs(GT_FIELD_ADDR)) && - (varTypeIsGC(parent->TypeGet()))) + if (parent->OperIs(GT_STOREIND) && addr->OperIs(GT_FIELD_ADDR) && varTypeIsGC(parent->TypeGet())) { parent->ChangeType(newType); } diff --git a/src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.cs b/src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.cs new file mode 100644 index 00000000000000..9089fb0e334314 --- /dev/null +++ b/src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.cs @@ -0,0 +1,86 @@ +// 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.Reflection; +using System.Diagnostics; +using System.Runtime.CompilerServices; +using Xunit; + +class C1 +{ + public C1(int x) { a = x; b = x; } + public int a; + public int b; +} + +struct S1 +{ + public S1(C1 z) { c = z; } + public int a; + public int b; + public C1 c; +} + +public class Runtime_111922 +{ + [Fact] + public static int Problem() + { + S1 s = new S1(new C1(4)); + return 95 + SubProblem(1, s); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SubProblem(int x, S1 s) + { + s = new S1(new C1(5)); + + SideEffect(); + + C1 v = s.c; + return v.a; + } + + [Fact] + public static int Problem1() + { + S1 s = new S1(new C1(4)); + return 95 + SubProblem1(1, ref s); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SubProblem1(int x, ref S1 s) + { + s = new S1(new C1(5)); + + SideEffect(); + + C1 v = s.c; + return v.a; + } + + [Fact] + public static int Problem2() + { + S1 s = new S1(new C1(4)); + return 91 + SubProblem2(0, s) + SubProblem2(1, s); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SubProblem2(int x, S1 s) + { + if (x == 0) + { + s = new S1(new C1(5)); + } + + SideEffect(); + + C1 v = s.c; + return v.a; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void SideEffect() { } +} diff --git a/src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.csproj b/src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.csproj new file mode 100644 index 00000000000000..501217e4d86892 --- /dev/null +++ b/src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.csproj @@ -0,0 +1,9 @@ + + + None + True + + + + + From c52d50069815c3f3943fa78c5b997314a110ab22 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 24 Apr 2025 13:24:59 -0700 Subject: [PATCH 2/3] limit to implicit byrefs --- src/coreclr/jit/objectalloc.cpp | 6 ++--- .../ObjectStackAllocation/Runtime_111922.cs | 22 +++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index eba1604336ff0f..f22764a763ecb1 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -708,12 +708,12 @@ void ObjectAllocator::MarkEscapingVarsAndBuildConnGraph() continue; } - // We have to mark all struct params as escaping, because + // We have to mark all implicit byref params as escaping, because // their GC reporting is controlled by the caller // - if (lclDsc->lvIsParam && (lclDsc->lvType == TYP_STRUCT)) + if (lclDsc->lvIsParam && lclDsc->lvIsImplicitByRef) { - JITDUMP(" V%02u is a struct param\n", lclNum); + JITDUMP(" V%02u is an implicit byref param\n", lclNum); MarkLclVarAsEscaping(lclNum); continue; } diff --git a/src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.cs b/src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.cs index 9089fb0e334314..7e45e21098793c 100644 --- a/src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.cs +++ b/src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.cs @@ -81,6 +81,28 @@ static int SubProblem2(int x, S1 s) return v.a; } + [Fact] + public static int Problem3() + { + C1 c = new C1(6); + return 1 + SubProblem3(0, c, c, c, c, c, c, c, c) + SubProblem3(1, c, c, c, c, c, c, c, c); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int SubProblem3(int x, C1 c1, C1 c2, C1 c3, C1 c4, C1 c5, C1 c6, C1 c7, C1 c8) + { + if (x == 0) + { + c1 = new C1(7); + c8 = new C1(8); + } + + SideEffect(); + + return c1.a + c2.a + c3.a + c4.a + c5.a + c6.a + c7.a + c8.a; + } + + [MethodImpl(MethodImplOptions.NoInlining)] static void SideEffect() { } } From bfec91a3e08c352e0522f9ff43096a9a786327d9 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 24 Apr 2025 14:19:29 -0700 Subject: [PATCH 3/3] fix build --- src/coreclr/jit/objectalloc.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/jit/objectalloc.cpp b/src/coreclr/jit/objectalloc.cpp index f22764a763ecb1..f931946b82a3a4 100644 --- a/src/coreclr/jit/objectalloc.cpp +++ b/src/coreclr/jit/objectalloc.cpp @@ -708,6 +708,7 @@ void ObjectAllocator::MarkEscapingVarsAndBuildConnGraph() continue; } +#if FEATURE_IMPLICIT_BYREFS // We have to mark all implicit byref params as escaping, because // their GC reporting is controlled by the caller // @@ -717,6 +718,7 @@ void ObjectAllocator::MarkEscapingVarsAndBuildConnGraph() MarkLclVarAsEscaping(lclNum); continue; } +#endif // Parameters have unknown initial values. // OSR locals have unknown initial values.