-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[release/10.0] JIT: disable stack allocation for sites in CEA cloned regions #121860
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
base: release/10.0
Are you sure you want to change the base?
[release/10.0] JIT: disable stack allocation for sites in CEA cloned regions #121860
Conversation
…#121771) Our escape analysis assumes each stack allocation candidate node has a unique temp that has that allocation as its only assigned value. Conditional escape analysis may clone these nodes, creating a second allocation site assigning to the associated temp. This breaks the 1-1 assumption noted above. If those allocations do not escape then the JIT will stack allocate them, creating two distinct stack locals. In subsequent IR rewriting the JIT will then use the address of just one of the locals for all appearances of the temp, which is incorrect, and leads to the generated code possibly reading uninitialized stack slots. To fix this we disallow stack allocation for sites in blocks that were cloned, or their clones. (We can actually handle this case with better bookkeeping, but that is more involved). Fixes dotnet#121736.
|
PTAL @dotnet/jit-contrib |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR is a backport of #121771 to fix a JIT issue where conditional escape analysis (CEA) could silently generate incorrect code. The fix disables stack allocation for blocks that were cloned or are clones, as these violate the single-assignment assumption used by escape analysis.
Key changes:
- Adds tracking to distinguish original blocks from cloned blocks using
m_initialMaxBlockID - Implements
BlockIsCloneOrWasCloned()to identify problematic blocks - Updates
MorphAllocObjNodeHelper()to prevent stack allocation at cloned sites
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tests/JIT/opt/ObjectStackAllocation/Runtime_121736.csproj | Test project for the regression case |
| src/tests/JIT/opt/ObjectStackAllocation/Runtime_121736.cs | Regression test using LINQ operations that trigger the CEA cloning scenario |
| src/coreclr/jit/objectalloc.h | Adds m_initialMaxBlockID field and BlockIsCloneOrWasCloned() method declaration |
| src/coreclr/jit/objectalloc.cpp | Implements the fix: captures initial block ID, adds clone detection logic, and prevents stack allocation for cloned blocks |
JulieLeeMSFT
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
Failures look like #120577 |
|
There was a fix in #120736 but maybe this is a new case? |
|
/ba-g seeing recurrence of a supposedly fixed error |
Backport of #121771 to release/10.0
/cc @AndyAyersMS
Customer Impact
Reported by customer in #121736
Regression
An issue in a new optimization in .NET 10.
Escape analysis assumes each stack allocation candidate node has a unique temp that has that allocation as its only assigned value. Conditional escape analysis may clone these nodes, creating a second allocation site assigning to the same temp. This breaks the 1-1 assumption noted above and can lead to silent bad codegen.
To fix this we disallow stack allocation for sites in blocks that were cloned, or their clones.
Testing
Verified on the test case from the issue.
Risk
Low. Disables an optimization. No diffs for this in our SPMI testing.