JIT: fix retyping of BLK ops when parent local is retyped #116027
Conversation
Properly handle cases where we have updated the layout of a struct local (because its fields may now refer to stack allocated objects) and then store or load from just part of that local. Fixes dotnet#115979.
github-actions
bot
added
the
area-CodeGen-coreclr
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the retyping of BLK operations when a parent local’s layout changes, addressing issues related to partial updates of struct locals.
- Updated the object allocation logic in the JIT to correctly handle retyping for both store and load operations.
- Added a regression test in the ObjectStackAllocation test suite to capture the fixed behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tests/JIT/opt/ObjectStackAllocation/Runtime_115979.csproj | Added a new test project for the regression test. |
| src/tests/JIT/opt/ObjectStackAllocation/Runtime_115979.cs | Added a test case to validate the fixed retyping behavior. |
| src/coreclr/jit/objectalloc.cpp | Updated retyping logic for BLK ops in both store and load cases. |
| // If we are storing a struct, we may need to change the layout | ||
| // | ||
| if (retypeFields && parent->OperIs(GT_STORE_BLK)) | ||
| else if (retypeFields && parent->OperIs(GT_STORE_BLK)) |
There was a problem hiding this comment.
Consider adding a more detailed comment above this branch to clarify the specific conditions for adjusting the block layout during BLK store operations, especially for the case when only a portion of the local is updated.
Copilot uses AI. Check for mistakes.
| @@ -2219,19 +2239,54 @@ void ObjectAllocator::UpdateAncestorTypes( | |||
| { | |||
| // If we are loading from a GC struct field, we may need to retype the load | |||
| // | |||
| if (retypeFields && (varTypeIsGC(parent->TypeGet()))) | |||
| if (retypeFields) | |||
There was a problem hiding this comment.
It might be helpful to document the purpose of the 'didRetype' flag and the rationale for resetting 'retypeFields' to false in this load operation branch, to improve future maintainability and clarity.
Copilot uses AI. Check for mistakes.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@jakobbotsch PTAL No diffs. |
|
Failures seem unrelated, but are not flagged by build analysis. Going to look at some other recent PRs. |
|
@jakobbotsch ping |
Properly handle cases where we have updated the layout of a struct local (because its fields may now refer to stack allocated objects) and then store or load from just part of that local.
Fixes #115979.