-
Notifications
You must be signed in to change notification settings - Fork 5k
JIT: fix issues in field-wise escape analysis #114974
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -698,12 +698,27 @@ 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; | ||
} | ||
|
||
#if FEATURE_IMPLICIT_BYREFS | ||
// We have to mark all implicit byref params as escaping, because | ||
// their GC reporting is controlled by the caller | ||
// | ||
if (lclDsc->lvIsParam && lclDsc->lvIsImplicitByRef) | ||
{ | ||
JITDUMP(" V%02u is an implicit byref param\n", lclNum); | ||
MarkLclVarAsEscaping(lclNum); | ||
continue; | ||
} | ||
#endif | ||
|
||
// Parameters have unknown initial values. | ||
// OSR locals have unknown initial values. | ||
|
@@ -2047,8 +2062,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())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that the removal of the retypeFields condition is fully intentional so that all scenarios for retyping GT_STOREIND are correctly handled. Consider adding an inline comment to clarify this design decision for future maintainers. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||
{ | ||
parent->ChangeType(newType); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
// 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; | ||
} | ||
|
||
[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() { } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<DebugType>None</DebugType> | ||
<Optimize>True</Optimize> | ||
</PropertyGroup> | ||
<ItemGroup> | ||
<Compile Include="$(MSBuildProjectName).cs" /> | ||
</ItemGroup> | ||
</Project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only necessary for implicit byrefs, or truly all struct parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All. We need this any time the caller is going to report GC references for a parameter, since we can't alter how those GC slots are reported.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can similar issues happen for primitive parameters too then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems possible if they are passed in memory, and don't end up enregistered. Let me work up a test case. Luckily (I guess) parameter reassignment is rare.
I wonder if that might motivate us to make copies of some of these args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, seems it would be better to create new locals instead of doing this retyping. Probably even for all locals, not just parameter ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'm not sure... this fix "works" because it blocks any attempt to retype param struct fields. But it looks like for scalar params the callee reports them.
@dotnet/jit-contrib does anyone know whether GC reporting for memory params is done by the caller or callee? Does this vary by ABI (say on x86 where call args for call A can be pushed while executing call B?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the caller is responsible for implicit byrefs, otherwise callee. See
GCInfo::gcMakeRegPtrTable
.If had some way of forcing promotion we could probably lift this limitation too. But for now will just go with blocking implicit byrefs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implicit byrefs are retyped to pointers later by
fgRetypeImplicitByRefArgs
-- retyping their local here is not going to have any effect on that, so I guess in that sense it makes sense that there would be issues around this.I wonder if there are any similar issues around OSR locals... I suppose not because this retyping does not happen.