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/8.0-staging] JIT: Fixed incorrect reversed condition for GT #100372

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Mar 27, 2024

Backport of #92316 to release/8.0-staging

/cc @kunalspathak @TIHan

Customer Impact

  • Customer reported
  • Found internally

This was reported by a customer in #99954, where they were having a wrong functional behavior from a simple condition evaluation a <= b should have been evaluated to true but was evaluating to false, leading to having their code take different code path. In the example that customer provided, they were seeing an exception was thrown because of the issue and the correct behavior was that no exception should have been thrown. The circumstances for it to occur is rare though and would trigger under very specific conditions. The false block (the code follows the else should have been optimized in prior phases and should be empty when we were trying to optimize the code in question) and there are some other requirements like what type of blocks they jump to in order the problematic code path to get triggered.
 

Regression

  • Yes
  • No

The code has been there since dotnet/coreclr days and was introduced back in dotnet/coreclr#17733, but we recently saw it getting exposed.

Testing

The fix was verified on the customer provided example in #99954. The code has been there for a while and was not discovered until September 2023 when we found out and fixed it in .NET 9. However, we didn't back-ported to .NET 8 back then. The backport also adds a test case for this scenario.

Risk

Low: This is a rare scenario and we had it .NET 9 for a while without causing any problem.

@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 Mar 27, 2024
Copy link
Contributor

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

@kunalspathak
Copy link
Member

@dotnet/jit-contrib

@AndyAyersMS
Copy link
Member

@kunalspathak I would have classified this as low risk, we have had the fix in 9.0 for a while and it has not led to further issues.

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.

approved. we will take for consideration in 8.0.x

@jeffschwMSFT jeffschwMSFT added this to the 8.0.x milestone Mar 28, 2024
@rbhanda rbhanda modified the milestones: 8.0.x, 8.0.5 Mar 28, 2024
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 28, 2024
@ericstj
Copy link
Member

ericstj commented Apr 15, 2024

@jeffschwMSFT @AndyAyersMS @TIHan - please assess failures and merge today.

@AndyAyersMS
Copy link
Member

Build/test logs are gone, so will need to rerun.

@AndyAyersMS
Copy link
Member

going to bounce this as all the assets are gone too.

@AndyAyersMS AndyAyersMS reopened this Apr 15, 2024
@directhex
Copy link
Member

Looking good, @AndyAyersMS

@AndyAyersMS
Copy link
Member

Yep.

@AndyAyersMS AndyAyersMS merged commit aa7c7ff into release/8.0-staging Apr 15, 2024
123 of 130 checks passed
@jkotas jkotas deleted the backport/pr-92316-to-release/8.0-staging branch April 17, 2024 01:25
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

7 participants