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.

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

Add flags checks to BMI1 intrinsic lowering #66736

Merged
merged 3 commits into from
Mar 20, 2022
Merged

Add flags checks to BMI1 intrinsic lowering #66736

merged 3 commits into from
Mar 20, 2022

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Mar 16, 2022

Fixes #66709

On x86 longs are decomposed into two integer nodes. This bug was caused by one of those integer nodes being recognised as part of the blsi pattern, x & -x and transformed without the second node being included. On x86 the 64 bit version of the intrinsic is not available so the lowering of a 64 bit operation to the intrinsic is not possible.

This PR adds in flags checks on all nodes being removed to see if they have side effect and if they have the lowering is not attempted.

I have added the same checks to blsr and andn even though I have confirmed that they do not suffer from the same bug. This seemed prudent to me in case future jit changes expose a situation where it does become possible for those lowering to be affected.

/cc @dotnet/jit-contrib

@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 16, 2022
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 16, 2022
@ghost
Copy link

ghost commented Mar 16, 2022

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

Issue Details

Fixes #66709

On x86 longs are decomposed into two integer nodes. This bug was caused by one of those integer nodes being recognised as part of the blsi pattern, x & -x and transformed without the second node being included. On x86 the 64 bit version of the intrinsic is not available so the lowering of a 64 bit operation to the intrinsic is not possible.

This PR adds in flags checks on all nodes being removed to see if they have side effect and if they have the lowering is not attempted.

I have added the same checks to blsr and andn even though I have confirmed that they do not suffer from the same bug. This seemed prudent to me in case future jit changes expose a situation where it does become possible for those lowering to be affected.

/cc @dotnet/jit-contrib

Author: Wraith2
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@jakobbotsch
Copy link
Member

Can you add the regression test?

AndyAyersMS
AndyAyersMS reviewed Mar 16, 2022
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Can you also add the test case that uncovered this?

Feel free to add it to your existing tests, if that works out.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Mar 16, 2022

I've updated the bmi1 tests to have 64 bit overloads and verified that they fail on current main as expected.

SingleAccretion
SingleAccretion reviewed Mar 17, 2022
View reviewed changes
src/coreclr/jit/lowerxarch.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/lowerxarch.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/lowerxarch.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/lowerxarch.cpp Outdated Show resolved Hide resolved
@jakobbotsch
Copy link
Member

Failure is #66571.

@jakobbotsch jakobbotsch merged commit fa1f283 into dotnet:main Mar 20, 2022
@Wraith2 Wraith2 deleted the fix-bmi1-decomposition branch March 20, 2022 11:23
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
@Wraith2 @radekdoulik
Add flags checks to BMI1 intrinsic lowering (dotnet#66736)
a42e098 
* add flags checks to BMI1 intrinsic lowering to prevent decomposed longs in x84 from being altered

* update bmji1intrinsics tests
@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Apr 1, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Apr 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SingleAccretion SingleAccretion SingleAccretion approved these changes

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

@jakobbotsch jakobbotsch jakobbotsch 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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JIT: Incorrect optimization of x & -x on x86
4 participants
@Wraith2 @jakobbotsch @AndyAyersMS @SingleAccretion