JIT: enhance escape analysis to understand local struct fields #113977

AndyAyersMS · 2025-03-27T17:08:54Z

Continuation of #113141

Implement a very simplistic "field sensitive" analysis for gc structs where we model each struct as simply its gc field(s).

That is, given a gc struct G with GC field f, we model

G.f = ...

as an assignment to G and

   = G.f

as a read from G. Since we know G itself cannot escape, "escaping" of G means G.f can escape.

Note this conflates the fields of G: either they all escape or none of them do. We could make this more granular but it's not clear that doing so is worthwhile, and it requires more up-front work to determine how to track each field's status.

If the struct or struct fields are only consumed locally, we may be able to prove the gc referents don't escape.

This is a subset/elaboration of #112543 that does not try and reason interprocedurally.

Contributes to #104936 / #108913
Fixes #113236 (once the right inlines happen)

Continuation of dotnet#113141 Implement a very simplistic "field sensitive" analysis for gc structs where we model each struct as simply its gc field(s). That is, given a gc struct `G` with GC field `f`, we model ``` G.f = ... ``` as an assignment to `G` and ``` = G.f ``` as a read from `G`. Since we know `G` itself cannot escape, "escaping" of `G` means `G.f` can escape. Note this conflates the fields of `G`: either they all escape or none of them do. We could make this more granular but it's not clear that doing so is worthwhile, and it requires more up-front work to determine how to track each field's status. If the struct or struct fields are only consumed locally, we may be able to prove the gc referents don't escape. This is a subset/elaboration of dotnet#112543 that does not try and reason interprocedurally. Contributes to dotnet#104936 / dotnet#108913 Fixes dotnet#113236 (once the right inlines happen)

Copilot

Pull Request Overview

This pull request enhances the JIT escape analysis by introducing field‐sensitive handling of GC structs and expanding test coverage for various scenarios involving spans and struct escapes. Key changes include the addition of new GC struct definitions and related tests, multiple span capture/escape test methods, and adjustments to the allocation verification logic in the tests.

Files not reviewed (6)

src/coreclr/jit/compiler.h: Language not supported
src/coreclr/jit/jitconfigvalues.h: Language not supported
src/coreclr/jit/layout.cpp: Language not supported
src/coreclr/jit/layout.h: Language not supported
src/coreclr/jit/objectalloc.cpp: Language not supported
src/coreclr/jit/objectalloc.h: Language not supported

Comments suppressed due to low confidence (2)

src/tests/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs:476

The method 'SpanCaptureArray3' is defined but never invoked by any test. Consider adding a test call (via CallTestAndVerifyAllocation) or removing the method if it is not needed.

static int SpanCaptureArray3()

src/tests/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs:512

The 'SpanEscapeArrayReturn' method is defined but not used in any test invocation. Consider adding a corresponding test or removing the method if it is unnecessary.

static int SpanEscapeArrayReturn() => SpanEscapeArrayReturnHelper()[10];

Copilot · 2025-03-27T17:09:20Z

src/tests/JIT/opt/ObjectStackAllocation/ObjectStackAllocationTests.cs

+            g.a[0] = "Goodbye";
+            g.b[0] = "Hello";
+
+            ref string[] rs = ref g.b;


The use of a ref assignment between g.b and later g.a in a conditional block could benefit from a clarifying comment to explain the intended behavior.

AndyAyersMS · 2025-03-27T17:12:14Z

@dotnet/jit-contrib PTAL

This refreshes #113141 based on recent BV refactoring and other changes. I have not incorporated all the feedback from that PR yet, so feel free to re-raise.

SPMI should do a decent job here demonstrating impact. I also haven't yet looked through all the regressions in detail.

jakobbotsch · 2025-03-27T17:16:28Z

src/coreclr/jit/layout.cpp

+            {
+                gcType = TYPE_GC_BYREF;
+            }
+            SetGCPtr(startSlot + slot, gcType);
        }
    }
 }


Same comment as previously.. This feels awfully specific to escape analysis to add in the generic builder type.

AndyAyersMS · 2025-03-27T19:58:09Z

Current diffs.

Looks like there is a bug to sort out first.

AndyAyersMS · 2025-03-31T19:01:43Z

Still hitting some asserts on x86 locally (32 bit is a stress case because we try and track all pointer-sized types).

AndyAyersMS · 2025-04-01T16:12:05Z

Allowing TYP_I_IMPL in the connection graph creates some potential confusion. EG if we have

[000458] DA--G------                         *  STORE_LCL_VAR int    V44 tmp43        
[000457] n---G------                         \--*  IND       int   
[000456] -----------                            \--*  FIELD_ADDR byref  System.ReadOnlySpan`1[int]:_length
[000455] -----------                               \--*  LCL_ADDR  byref  V43 tmp42        [+0]

On 32 bit architectures this will add a connection graph edge V44 <- V43 since V43 is a struct with a GC field and we coalesce fields.

Now if V43's GC field ends up being assigned a mixture of stack and heap allocations, we will retype the GC field in V43 as TYP_BYREF. And then given the connection above, we would deduce V44 is also TYP_BYREF and would try and rewrite V44 as TYP_BYREF, which is clearly nonsensical.

To fix this I have extracted the var retyping as a prepass, so that when we go to write the IR all vars have their "final" types. And we now will not retype any non-GC vars.

We should look into the implications of not modelling TYP_I_IMPL.

AndyAyersMS · 2025-04-03T01:54:17Z

We should look into the implications of not modelling TYP_I_IMPL.

Not too bad -- handled in #114130

@jakobbotsch I am tempted to defer futher refactoring until this is merged, so I can do that as zero diff on top. Thoughts?

jakobbotsch · 2025-04-03T11:16:23Z

@jakobbotsch I am tempted to defer futher refactoring until this is merged, so I can do that as zero diff on top. Thoughts?

Sounds fine to me. There's also some more refactoring that I think would be worth it, like the change to use ClassLayout more widely and the change to get rid of getTypeForBoxOnStack in favor of a custom layout.

jakobbotsch · 2025-04-03T11:34:28Z

src/coreclr/jit/layout.cpp

+        if (layoutName != nullptr)
+        {
+            size_t len = strlen(prefix) + strlen(layoutName) + 1;
+            newName    = m_compiler->getAllocator(CMK_DebugOnly).allocate<char>(len);
+            sprintf_s(newName, len, "%s%s", prefix, layoutShortName);
+        }
+
+        if (layoutShortName != nullptr)
+        {
+            size_t len   = strlen(prefix) + strlen(layoutName) + 1;
+            newShortName = m_compiler->getAllocator(CMK_DebugOnly).allocate<char>(len);
+            sprintf_s(newShortName, len, "%s%s", prefix, layoutShortName);
+        }


This can use printfAlloc.

Yes, will do that in the follow-up.

jakobbotsch · 2025-04-03T11:35:35Z

src/coreclr/jit/layout.cpp

+    // Do the normal compatibility check first, when possible to do so.
+    //
+    if ((IsCustomLayout() == layout->IsCustomLayout()) || (!HasGCPtr() && !layout->HasGCPtr()))
+    {
+        const bool areCompatible = AreCompatible(this, layout);
+
+        if (areCompatible)
+        {
+            return true;
+        }
+    }


Doesn't the logic below subsume this check?

I wanted to make this a superset of what AreCompatible does but that ended up being a bit clunky since I need to guard the call to avoid asserts.

I can either just give up on that idea or else give AreCompatible and optional early out to avoid asserting and just return false.

What is the assert?

I would probably vote to just take the small amount of duplication. Seems simpler to reason about the operation that way.

runtime/src/coreclr/jit/layout.cpp

Lines 712 to 713 in f79c098

assert(clsHnd1 != NO_CLASS_HANDLE);

assert(clsHnd2 != NO_CLASS_HANDLE);

Without the guarding we can get here with a custom (byref) source and a class-based (ref) dest. The "early" out would be just before this, since in this situation an exact gc slot compare will fail; we need the more relaxed one that allows mixed types.

I think that assert is just outdated, it should have been removed as part of #112064 since we can now have layouts without class handles but with GC pointers

In that case I can just remove the guarding and call it unconditionally.

jakobbotsch · 2025-04-03T11:38:43Z

src/coreclr/jit/objectalloc.cpp

+                if (lhs == tree)
+                {
+                    if (rhs->IsIntegralConst(0))
+                    {
+                        rhs->ChangeType(newType);
+                    }
+                }
+                else if (rhs == tree)
+                {
+                    if (lhs->IsIntegralConst(0))
+                    {
+                        lhs->ChangeType(newType);
+                    }
+                }


What about other objects, like frozen objects?

Seems like it could be possible (at least EQ/NE).

So is a fix needed? It seems like object stack allocation can now start producing mixed TYP_REF/TYP_BYREF compares. Does it mean the rest of the JIT will have to be taught to handle these kind of mixes?

Good question.

I think it is reasonable to assert that relops (and maybe some other kinds of binops) should have same-typed operands. But (as you noted earlier) it also seems wrong to forcibly retype unrelated computation.

We are running into cases where the current type system can't really express what we are able to represent. A reference to an on-stack object is still an object reference, but we must use TYP_I_IMPL to describe its referent, and an operand that might refer to a stack or heap object likewise must now be TYP_BYREF.

Introducing new types doesn't seem like the right answer either, though maybe something more like subtypes of TYP_REF is appropriate? Say TYP_REF_HEAP, TYP_REF_STACK, TYP_REF_ANYWHERE, etc... But given the widespread knowledge of the basic types any change here would be very messy.

Yes, I think that's part of the broader IR change where object stack allocation introduces implicit conversions between TYP_REF and TYP_BYREF that other things in the JIT may not be expecting. I still worry we will end up with a bug tail around these mismatches.

I wonder if the "special GC segment" idea would also give us a way to avoid these typing woes in the JIT. For example, I could imagine only stack allocating and retyping accesses for objects where we can see all uses, and otherwise use the special GC segment and keep accesses TYP_REF typed. Still comes with some questions for write barriers, though.

jakobbotsch · 2025-04-03T14:06:38Z

Should we run jitstress/gcstress for this?

azure-pipelines · 2025-04-08T23:04:29Z

Azure Pipelines successfully started running 3 pipeline(s).

AndyAyersMS · 2025-04-09T16:50:20Z

Failures all seem unrelated. But let's rerun since some of it was cleaned up.

AndyAyersMS · 2025-04-09T16:50:50Z

/azp run runtime-coreclr jitstress, runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr libraries-pgo

azure-pipelines · 2025-04-09T16:51:09Z

Azure Pipelines successfully started running 3 pipeline(s).

AndyAyersMS · 2025-04-09T22:48:22Z

CLR_Tools_Tests failure is Mono.Linker.Tests.TestCases.All.DataFlow failing consistently #114426.
GC Stress is osx timeout.
x86: not sure, seems unrelated.

AndyAyersMS · 2025-04-11T00:54:21Z

@jakobbotsch this is the "simpler" version where we don't try and track any scalar locals that are address-taken.

AndyAyersMS · 2025-04-11T18:39:39Z

/azp run runtime-coreclr jitstress, runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr libraries-pgo

azure-pipelines · 2025-04-11T18:40:04Z

Azure Pipelines successfully started running 3 pipeline(s).

AndyAyersMS · 2025-04-12T01:29:53Z

jitstress failure is timeout
osx gcstress likely also will timeout.

AndyAyersMS · 2025-04-14T14:27:46Z

@jakobbotsch this latest has the &scalar fixes in it.

jakobbotsch · 2025-04-15T13:56:14Z

src/coreclr/jit/compiler.h

+    // Change the layout to one that may not be compatible.
+    void ChangeLayout(ClassLayout* layout)
+    {
+        assert(varTypeIsStruct(lvType));
+        m_layout = layout;
+    }


Replace GrowBlockLayout with this?

Not sure ... neither is widely used, both likely get inlined and evaporate in release.

jakobbotsch · 2025-04-15T13:56:51Z

src/coreclr/jit/layout.cpp

+//    layout1 - the first layout (copy destination)
+//    layout2 - the second layout (copy source)


will address this in the follow-up.

jakobbotsch · 2025-04-15T14:04:38Z

src/coreclr/jit/objectalloc.cpp

+                // Is this a store to a field of a local struct...?
+                //
+                if (parent->OperIs(GT_STOREIND))
+                {
+                    // Are we storing to a local field?
+                    //
+                    if (addr->OperIs(GT_FIELD_ADDR))
+                    {
+                        // Simple check for which local.
+                        //
+                        GenTree* const base = addr->AsOp()->gtGetOp1();
+
+                        if (base->OperIs(GT_LCL_ADDR))
+                        {
+                            unsigned const   dstLclNum = base->AsLclVarCommon()->GetLclNum();
+                            LclVarDsc* const dstDsc    = comp->lvaGetDesc(dstLclNum);
+
+                            if (IsTrackedLocal(dstLclNum))
+                            {
+                                JITDUMP("... local.field store\n");
+                                // Add an edge to the connection graph.
+                                AddConnGraphEdge(dstLclNum, lclNum);
+                                canLclVarEscapeViaParentStack = false;
+                            }
+                        }
+                    }


I thought you were going with the more conservative approach, but evidently not completely?

Conservative for non-struct locals, but not for struct locals.

If it turns out we have a field addr on a non-struct local, the lcl addr will have caused that local to escape.

hez2010 · 2025-04-16T00:22:07Z

Will fields of a stack-allocated ref object be considered as "fields of local structs" here as well?

AndyAyersMS · 2025-04-16T01:08:27Z

Will fields of a stack-allocated ref object be considered as "fields of local structs" here as well?

No, this is strictly for local struct fields.

For fields of stack allocated objects we can do something similar, but we'll need to distinguish between the object and its fields and allocate separate connection graph nodes for the fields.

And to do that we need to know how many allocation sites there are up front, which either entails an IL traversal prepass or else flagging the newobj/newarr temps during importation so we can count them cheaply when we get into the object allocation phase.

Jakob and I have been kicking around the idea that we should run parts of local morph here to put local addresses into simpler forms so perhaps that gives us motivation to look at running a prepass...

Copilot AI review requested due to automatic review settings March 27, 2025 17:08

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 27, 2025

dotnet-policy-service bot assigned AndyAyersMS Mar 27, 2025

Copilot AI reviewed Mar 27, 2025

View reviewed changes

jakobbotsch reviewed Mar 27, 2025

View reviewed changes

AndyAyersMS mentioned this pull request Mar 27, 2025

JIT: enhance escape analysis to understand local struct fields #113141

Closed

build-analysis bot mentioned this pull request Mar 27, 2025

System.Net.Requests test timeout #113883

Closed

This was referenced Mar 27, 2025

[QUIC & HTTP/3] Handshake Timeout on tests #104426

Open

System.Net.Quic tests timeout #107761

Open

AndyAyersMS added 3 commits March 30, 2025 18:06

handle non-struct local indirs better

b6bf10b

only consider loads of tracked types

70e2d14

restrict general indirs to byrefs and structs

e0432bd

AndyAyersMS added 2 commits March 31, 2025 14:53

more null retyping (relops)

12f4c56

split out local var retyping from IR updates

adf2baf

merge main

d3727d3

jakobbotsch reviewed Apr 3, 2025

View reviewed changes

Merge branch 'main' into LocalStructFieldWiseEscapeAnalysis

fc0f13a

try and simplify handling of address-taken scalars

be237f6

cleanup; format

105ea69

build-analysis bot mentioned this pull request Apr 11, 2025

Android emulator not booting completely on Helix queue dotnet/dnceng#1448

Open

3 tasks

Merge branch 'main' into LocalStructFieldWiseEscapeAnalysis

9fa1a40

jakobbotsch self-requested a review April 14, 2025 14:35

jakobbotsch reviewed Apr 15, 2025

View reviewed changes

jakobbotsch approved these changes Apr 15, 2025

View reviewed changes

AndyAyersMS merged commit 212b120 into dotnet:main Apr 15, 2025
150 of 158 checks passed

AndyAyersMS mentioned this pull request Apr 15, 2025

JIT: Escape analysis cleanup #114716

Merged

This was referenced Mar 23, 2025

Stack Allocation Enhancements #104936

Open

JIT: De-abstraction in .NET 10 #108913

Open

	assert(clsHnd1 != NO_CLASS_HANDLE);
	assert(clsHnd2 != NO_CLASS_HANDLE);

		// layout1 - the first layout (copy destination)
		// layout2 - the second layout (copy source)

JIT: enhance escape analysis to understand local struct fields #113977

JIT: enhance escape analysis to understand local struct fields #113977

Conversation

AndyAyersMS commented Mar 27, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Copilot AI Mar 27, 2025

Choose a reason for hiding this comment

AndyAyersMS commented Mar 27, 2025

Choose a reason for hiding this comment

AndyAyersMS commented Mar 27, 2025

AndyAyersMS commented Mar 31, 2025

AndyAyersMS commented Apr 1, 2025

AndyAyersMS commented Apr 3, 2025

jakobbotsch commented Apr 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Apr 3, 2025

azure-pipelines bot commented Apr 8, 2025

AndyAyersMS commented Apr 9, 2025

AndyAyersMS commented Apr 9, 2025

azure-pipelines bot commented Apr 9, 2025

AndyAyersMS commented Apr 9, 2025

AndyAyersMS commented Apr 11, 2025

AndyAyersMS commented Apr 11, 2025

azure-pipelines bot commented Apr 11, 2025

AndyAyersMS commented Apr 12, 2025

AndyAyersMS commented Apr 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hez2010 commented Apr 16, 2025

AndyAyersMS commented Apr 16, 2025