[release/8.0] JIT: Fix physical promotion creating overlapping local uses #91088
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Physical promotion could in some cases create uses overlapping illegally with defs when faced with seemingly last uses of structs. This is a result of a mismatch between our model for liveness and the actual model of local uses in the backend. In the actual model, the uses of LCL_VARs occur at the user, which means that it is possible for there to be no place at which to insert IR between to local uses. The example looks like the following. Physical promotion would be faced with a tree like ``` ▌ CALL void Program:Foo(Program+S,Program+S) ├──▌ LCL_VAR struct<Program+S, 4> V01 loc0 └──▌ LCL_VAR struct<Program+S, 4> V01 loc0 ``` When V01 was fully promoted, both of these are logically last uses since all state of V01 is stored in promoted field locals. Because of that we would make the following transformation: ``` ▌ CALL void Program:Foo(Program+S,Program+S) ├──▌ LCL_VAR struct<Program+S, 4> V01 loc0 (last use) └──▌ COMMA struct ├──▌ STORE_LCL_FLD int V01 loc0 [+0] │ └──▌ LCL_VAR int V02 tmp0 └──▌ LCL_VAR struct<Program+S, 4> V01 loc0 (last use) ``` This creates an illegally overlapping use and def; additionally, it is correct only in a world where the store actually would happen between the two uses. It is also moderately dangerous to mark both of these as last uses given the implicit byref transformation. The fix is to avoid marking a struct use as a last use if we see more struct uses in the same statement. Fix #91056
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsBackport of #91058 to release/8.0 /cc @jakobbotsch Customer ImpactTestingRiskIMPORTANT: If this backport is for a servicing release, please verify that:
|
3 tasks
AndyAyersMS
approved these changes
Aug 25, 2023
jeffschwMSFT
approved these changes
Aug 25, 2023
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #91058 to release/8.0
/cc @jakobbotsch
Customer Impact
In certain cases when a large struct variable is passed multiple times to the same method, it is possible for the JIT to generate bad code on x86.
Testing
Regression test included.
Risk
Low.