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] Fix Issue #105820 #106656

Merged
merged 3 commits into from
Aug 21, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 19, 2024

Backport of #106655 to release/9.0

/cc @TIHan @github-actions[bot]

Customer Impact

  • Customer reported
  • Found internally

Correctness issue with the HW intrinsic Bmi1.BitFieldExtract when used as part of a conditional. test eax eax was not being emitted before the jl instruction.

Regression

  • Yes
  • No

Testing

Fuzzlyn found the fix in its code generation. This was missed due to no stress testing against these APIs; Fuzzlyn generates code in a variety of scenarios that are not common to end-users. No tests were added as it appeared to be hit commonly when Fuzzlyn was generating code.

Risk

Low, the fix was straightforward and simple. Additional checks such as DoesWriteSignFlag(lastIns) && DoesWriteZeroFlag(lastIns) && DoesWriteParityFlag(lastIns) were used to help determine if the conditional flags were reset to zero or not.

@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 Aug 19, 2024
Copy link
Contributor

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

@carlossanlop
Copy link
Member

carlossanlop commented Aug 20, 2024

@TIHan there's no main PR for this, the original PR was closed. I see my mistake. It was first backported to 9.0, then backported to RC1, and the 9.0 closed. Good.

Also, has this been approved by Tactics already? We're quite late in the process.

@JulieLeeMSFT
Copy link
Member

I think his intention was to backport to RC2. I will talk to @TIHan to fix it.

@TIHan there's no main PR for this, the original PR was closed. I see my mistake. It was first backported to 9.0, then backported to RC1, and the 9.0 closed. Good.

Also, has this been approved by Tactics already? We're quite late in the process.

@carlossanlop carlossanlop changed the title [release/9.0] [release/9.0-rc1] Fix Issue #105820 [release/9.0] Fix Issue #105820 Aug 20, 2024
@carlossanlop
Copy link
Member

Okay, apologies for the confusion. The title kept the [release/9.0-rc1] bit from the previous backport so it was confusing me. This PR is already targeting the release/9.0 branch, which is good (that's RC2).

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. please get a code review. we can merge when ready.

@jeffschwMSFT
Copy link
Member

@TIHan can you look at the failing CI and let me know when this is ready to merge?

@TIHan
Copy link
Contributor

TIHan commented Aug 21, 2024

Yes, will do.

@TIHan
Copy link
Contributor

TIHan commented Aug 21, 2024

Failures are unrelated to this change since all of them are arm related. There is a x86 installer build failure but is also unrelated.

So it can be merged.

@jeffschwMSFT
Copy link
Member

/ba-g failures are unrelated to change

@jeffschwMSFT jeffschwMSFT added the Servicing-approved Approved for servicing release label Aug 21, 2024
@jeffschwMSFT jeffschwMSFT merged commit a80d1c1 into release/9.0 Aug 21, 2024
10 of 12 checks passed
@jkotas jkotas deleted the backport/pr-106655-to-release/9.0 branch August 24, 2024 04:47
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 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.

5 participants