Skip to content

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

Merged
merged 4 commits into from
Apr 25, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions src/coreclr/jit/objectalloc.cpp
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;
}
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

@jakobbotsch jakobbotsch Apr 24, 2025

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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?)

Copy link
Member Author

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.

Copy link
Member

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.

#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()))
Copy link
Preview

Copilot AI Apr 23, 2025

Choose a reason for hiding this comment

The 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.

{
parent->ChangeType(newType);
}
108 changes: 108 additions & 0 deletions src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.cs
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() { }
}
9 changes: 9 additions & 0 deletions src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.csproj
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>
Loading
Oops, something went wrong.