JIT: don't dead store OSR exposed locals#89867
JIT: don't dead store OSR exposed locals#89867JulieLeeMSFT merged 1 commit intodotnet:release/7.0-stagingfrom
Conversation
Backport of dotnet#89743 to 7.0. The local var ref count for OSR locals can be misleading, since some of the appearances may be in the "tier0" parts of the methods and won't be visible once we trim the OSR method down to just the part we're going to compile. Fix by giving OSR-exposed locals an implicit ref count. Fixes dotnet#89666.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsBackport of #89743 to 7.0. Customer ImpactFixes customer reported issue #89666. Methods with loops might unexpectedly start running incorrectly after some time, because the runtime/JIT transitioned execution to a new native version via OSR, and in some rare cases the OSR method will not properly update a local variable. TestingVerified test from #89666 case fixed, added new test based on the underlying problem. SPMI screening of this fix in main showed minimal diffs and no instances of the problem. RiskLow, targeted fix for OSR that disable some optimizations. The local var ref count for OSR locals can be misleading, since some of the appearances may be in the "tier0" parts of the methods and won't be visible once we trim the OSR method down to just the part we're going to compile. Fix by giving OSR-exposed locals an implicit ref count; this prevents the JIT from inferring that a store to a local is not needed because the stored value cannot be read.
|
|
@jakobbotsch PTAL |
jeffschwMSFT
left a comment
jeffschwMSFT
left a comment
There was a problem hiding this comment.
approved. we will take for consideration in 7.0.x
|
Friendly reminder: if you want this servicing fix to be included in the September 2023 Release, you'll have to merge this PR before August 14th. |
jeffschwMSFT
added
Servicing-approved
|
@JulieLeeMSFT can you take a look at the CI failures? Once we understand them we can merge. |
|
There is only 1 failure on mono test, which is not related to this PR. It is a known issue. Merging this PR. |
ghost
locked as resolved and limited conversation to collaborators
5 participants
Backport of #89743 to 7.0.
Customer Impact
Fixes customer reported issue #89666.
Methods with loops might unexpectedly start running incorrectly after some time, because the runtime/JIT transitioned execution to a new native version via OSR, and in some rare cases the OSR method will not properly update a local variable.
Testing
Verified test from #89666 case fixed, added new test based on the underlying problem. SPMI screening of this fix in main showed minimal diffs and no instances of the problem.
Risk
Low, targeted fix for OSR that disable some optimizations.
The local var ref count for OSR locals can be misleading, since some of the appearances may be in the "tier0" parts of the methods and won't be visible once we trim the OSR method down to just the part we're going to compile.
Fix by giving OSR-exposed locals an implicit ref count; this prevents the JIT from inferring that a store to a local is not needed because the stored value cannot be read.