Skip to content
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

[release/9.0-staging] JIT: fix local assertion prop error for partial local comparisons #112539

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 13, 2025

Backport of #112506 to release/9.0-staging

/cc @AndyAyersMS

Customer Impact

  • Customer reported
  • Found internally

We got a report of incorrect behavior in 9.0 on github: #111352

This is silent bad code and could impact customers unexpectedly.

Regression

  • Yes
  • No

Introduced by #94741. We enabled more aggressive assertion propagation in the JIT "morph" phase, and one of the new classes of assertions could incorrectly draw inferences from operations on just the low 32 bits of long variables.

For example; if the program had code like

long x = ...;
long y = ...;
if ((int)x == (int)y)
{
     Console.WriteLine($"{x},{y}");
}

Then the JIT would mistakenly optimize the WriteLine to WriteLine($"{x},{x}");

Testing

Verified on a private repo case from the user that opened the issue, and extracted the problematic pattern and added a new test case.

Risk

Low. This optimization was enabled in our mainline in Dec 2023 and has just surfaced now. Also, no SPMI hits for this pattern.

If a JTRUE comparison only involves part of a local value we cannot make assertions
about the local as a whole.

Fixes #111352.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 13, 2025
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@AndyAyersMS
Copy link
Member

cc @dotnet/jit-contrib @jeffschwMSFT

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. please get a code review. we will take this for consideration in 9.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Feb 13, 2025
@jeffschwMSFT jeffschwMSFT added this to the 9.0.x milestone Feb 13, 2025
@rbhanda rbhanda modified the milestones: 9.0.x, 9.0.4 Feb 18, 2025
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 18, 2025
@jeffschwMSFT jeffschwMSFT merged commit 8a346d2 into release/9.0-staging Feb 28, 2025
104 of 106 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants