JIT: fix issues in field-wise escape analysis #114974
Conversation
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 dotnet#111922.
ghost
added
the
area-CodeGen-coreclr
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues in field‐wise escape analysis by ensuring that GC struct parameters are marked as escaping and that GT_STOREIND is retyped using the stored data’s new type.
- Fix GC struct parameters handling by marking them as escaping instead of retyping.
- Always use the stored data’s new type for GT_STOREIND operations.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.cs | Added tests to validate behavior related to escape analysis. |
| src/coreclr/jit/objectalloc.cpp | Updated escape marking logic and revised retyping condition for GT_STOREIND. |
Files not reviewed (1)
- src/tests/JIT/opt/ObjectStackAllocation/Runtime_111922.csproj: Language not supported
| @@ -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())) | |||
There was a problem hiding this comment.
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.
src/coreclr/jit/objectalloc.cpp
Outdated
| // 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)) |
There was a problem hiding this comment.
Confirm that marking all struct parameters as escaping aligns with the GC reporting requirements; an inline note could help clarify how this change impacts struct parameter handling.
Copilot uses AI. Check for mistakes.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@jakobbotsch PTAL (revised version of #114968) Modest number of good diffs from the retyping fix. |
|
I believe the failures are unrelated. |
| { | ||
| JITDUMP(" V%02u is a struct param\n", lclNum); | ||
| MarkLclVarAsEscaping(lclNum); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Is this only necessary for implicit byrefs, or truly all struct parameters?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can similar issues happen for primitive parameters too then?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
|
@jakobbotsch I revised this to just exclude implicit byrefs; the callee reports gc fields for all the other param cases. |
Two fixes:
Fixes #111922.