JIT: revisions to struct field escape analysis #116124
Conversation
Peel off some changes from the more general field-wise escape analysis: * make sure we have modelled all relevant stores. Change local store tracking to use the new model. * keep track of which temps will represent stack allocated arrays and do suitable retyping * reject "known escaping" allocations earlier (eg finalizable objects) * handle local fields (rare but can exist in importer IR) * add the field test cases; all of these stack allocate for now This generally is able to classify more locals as non-GC.
github-actions
bot
added
the
area-CodeGen-coreclr
There was a problem hiding this comment.
Pull Request Overview
This PR refines the JIT’s struct field escape analysis to better model field stores, improve local variable retyping, and distinguish between stack allocations for objects and arrays.
- Introduces a new StoreInfo struct and updates NodeToIndexMap to use it.
- Splits the heap-local-to-stack mapping into separate object and array maps.
- Revises allocation classification with the introduction of a new OAT_NEWOBJ_HEAP type and adjusts connection graph modeling.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tests/JIT/opt/ObjectStackAllocation/Fields.csproj | Adds project configuration for better isolation and optimization. |
| src/coreclr/jit/objectalloc.h | Adds StoreInfo struct and updates mapping types for escape analysis. |
| src/coreclr/jit/objectalloc.cpp | Revises allocation logic, retyping, and connection graph edge handling. |
| src/coreclr/jit/gentree.h | Removes the GTF_VAR_CONNECTED flag in accordance with the new model. |
Comments suppressed due to low confidence (1)
src/coreclr/jit/gentree.h:452
- Ensure that the removal of GTF_VAR_CONNECTED is consistent across the codebase so that modules expecting this flag are updated accordingly.
GTF_VAR_CONNECTED = 0x00100000, // GT_STORE_LCL_VAR -- this store was modelled in the connection graph during escape analysis
| assert(clsHnd != NO_CLASS_HANDLE); | ||
| const bool isValueClass = comp->info.compCompHnd->isValueClass(clsHnd); | ||
| bool const canBeOnStack = isValueClass || comp->info.compCompHnd->canAllocateOnStack(clsHnd); | ||
| allocType = canBeOnStack ? OAT_NEWOBJ : OAT_NEWOBJ_HEAP; |
There was a problem hiding this comment.
[nitpick] Consider adding documentation that explains the distinction between OAT_NEWOBJ and OAT_NEWOBJ_HEAP to clarify when each allocation type is chosen.
Copilot uses AI. Check for mistakes.
| @@ -2004,9 +2180,10 @@ void ObjectAllocator::AnalyzeParentStack(ArrayStack<GenTree*>* parentStack, unsi | |||
| } | |||
| } | |||
|
|
|||
| if (canLclVarEscapeViaParentStack) | |||
| if (canLclVarEscapeViaParentStack && !CanIndexEscape(lclIndex)) | |||
There was a problem hiding this comment.
[nitpick] Add a comment to clarify the rationale for including the '!CanIndexEscape(lclIndex)' check, which would help maintain code clarity.
Copilot uses AI. Check for mistakes.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@jakobbotsch PTAL SPMI will show many missing contexts here due to missing |
|
/azp run Fuzzlyn |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Looks like fuzzlyn sees more |
|
/azp run Fuzzlyn |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/ba-g unrelated test failure without a log |
Peel off some changes from the more general field-wise escape analysis:
This generally is able to classify more locals as non-GC.