Skip to content

[release/6.0] Disable folding small-typed returned indirections #82846

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.

Sign up for GitHub

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

Jump to bottom
Merged
carlossanlop merged 4 commits into from
Mar 10, 2023
Merged

[release/6.0] Disable folding small-typed returned indirections #82846

carlossanlop merged 4 commits into from
Mar 10, 2023

Conversation

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Mar 1, 2023 •
edited by jakobbotsch
Loading

/cc @jakobbotsch

Customer Impact

The BitConverter.HalfToInt16Bits method is miscompiled by the JIT in .NET 6. Customer reported and asked for backport in #82680.

.NET 7 is not impacted -- it was fixed there by #64881.

Testing

Regression test included.

Risk

Low -- targeted fix that disables the incorrect optimization in this specific scenario.

@jakobbotsch
Normalize returns after optimizing returned indirs
611bb2d 
This ensures we normalize returned indirs of locals, even when we fold
it into an access of a promoted local's field. This may change a signed
indir into access of an unsigned field, which may need a sign-extending
cast to be inserted.

It is not ideal that fgMorphRetInd can 'lose' this information, but
given that it is a specialized optimization it seems the simplest
solution is to just rely on the follow-up normalization.

Fix #61359
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 1, 2023
@ghost
Copy link

ghost commented Mar 1, 2023

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

Issue Details

Backport of #64881 to release/6.0

/cc @jakobbotsch

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@jakobbotsch jakobbotsch added this to the 6.0.x milestone Mar 1, 2023
@runfoapp runfoapp bot mentioned this pull request Mar 1, 2023
Closed
@jakobbotsch
Copy link
Member

jakobbotsch commented Mar 1, 2023 •
edited
Loading

This needs a different fix in .NET 6 because the fix being backported here actually relies on #59645 to work, and that one isn't in .NET 6.

Edit: Replaced with a different fix that disables the optimization in the problematic case.

@jakobbotsch jakobbotsch requested a review from AndyAyersMS March 1, 2023 23:17
@jakobbotsch jakobbotsch changed the title [release/6.0] Normalize returns after optimizing returned indirs [release/6.0] Disable folding small-typed returned indirections Mar 1, 2023
@build-analysis build-analysis bot mentioned this pull request Mar 2, 2023
Closed
@carlossanlop
Copy link
Contributor

carlossanlop commented Mar 8, 2023

@jakobbotsch when this PR is ready, please add the servicing-consider label and send the email to Tactics requesting approval.

Do any of the CI failures look concerning?

@build-analysis build-analysis bot mentioned this pull request Mar 9, 2023
Closed
@jakobbotsch jakobbotsch added the Servicing-consider Issue for next servicing release review label Mar 9, 2023
@jakobbotsch
Copy link
Member

jakobbotsch commented Mar 9, 2023

Do any of the CI failures look concerning?

Don't think so, they were all in Mono legs that this shouldn't affect. I had to merge this anyway so let's see the repeated CI run.

jeffschwMSFT
jeffschwMSFT approved these changes Mar 9, 2023
View reviewed changes
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 6.0.x

@carlossanlop
Copy link
Contributor

carlossanlop commented Mar 10, 2023

Approved by Tactics via email.
Signed off by area owners.
No OOB changes neede for this (native).
CI failures look unrelated.
Ready to merge. :shipit:

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 10, 2023
@carlossanlop carlossanlop modified the milestones: 6.0.x, 6.0.16 Mar 10, 2023
@carlossanlop carlossanlop merged commit cbfaba5 into release/6.0 Mar 10, 2023
@carlossanlop carlossanlop deleted the backport/pr-64881-to-release/6.0 branch March 10, 2023 00:23
@build-analysis build-analysis bot mentioned this pull request Mar 10, 2023
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jeffschwMSFT jeffschwMSFT jeffschwMSFT approved these changes

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

Assignees

No one assigned

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

Milestone

6.0.16

Development

Successfully merging this pull request may close these issues.

5 participants

@jakobbotsch @carlossanlop @jeffschwMSFT @AndyAyersMS