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

[JIT] Always transform AND(X, CNS(-1)) to X #82276

Merged
merged 8 commits into from
Feb 23, 2023

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Feb 17, 2023

Description

We should always try to lower AND(X, CNS(-1)) to X if possible. We do this transformation in lowering to pick up this pattern as a result of decomposing longs on 32bit archs.

@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 17, 2023
@ghost ghost assigned TIHan Feb 17, 2023
@ghost
Copy link

ghost commented Feb 17, 2023

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

Issue Details

We can always skip and reg0, -1, if the target type is TYP_INT, on X86 as the result will never be different.

Author: TIHan
Assignees: TIHan
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member

Can this optimization be done in lowering instead so that all targets benefit equally? Are there any benefits to the existing x64 peephole for doing the optimization in codegen?

@tannergooding
Copy link
Member

What's leading to the x & ~0 in the first place? Is this something specific that's being introduced in lowering or is it a more general missing opt in morph/constant folding?

@TIHan
Copy link
Contributor Author

TIHan commented Feb 17, 2023

@tannergooding @jakobbotsch

Good points, since we don't have to worry about the upper 32 bits, we could do the transform earlier. I have an idea where.

@jakobbotsch
Copy link
Member

since we don't have to worry about the upper 32 bits, we could do the transform earlier

Even on 64-bit we can do the optimization for TYP_INT GT_AND on IR nodes and I'd be curious to see if the peephole isn't just a more conservative version of that.

@TIHan
Copy link
Contributor Author

TIHan commented Feb 17, 2023

Even on 64-bit we can do the optimization

I don't think we can - I remember trying to do (x & -1) optimization in an earlier phase before; ran into issues of the upper 32bits, which is why I did it as a peephole.

However, we could definitely do a TYP_LONG version of GT_AND and make sure the constant is a TYP_LONG of -1.

@jakobbotsch
Copy link
Member

I don't think we can - I remember trying to do (x & -1) optimization in an earlier phase before; ran into issues of the upper 32bits, which is why I did it as a peephole.

I'd be interested to see the example, that certainly sounds odd or like there is a bug somewhere else.

@TIHan
Copy link
Contributor Author

TIHan commented Feb 17, 2023

So, we definitely should do the transformation in lowering for x86 because we catch cases of (x & -1) after decomposing longs.

@TIHan
Copy link
Contributor Author

TIHan commented Feb 21, 2023

@dotnet/jit-contrib This is ready PTAL @BruceForstall

@BruceForstall
Copy link
Member

Diffs -- significant asm diffs on x86, a few on x64

@TIHan TIHan changed the title [JIT] X86 - Always skip emitting 'and reg0, -1' on x86 [JIT] Always transform AND(X, CNS(-1)) to X Feb 22, 2023
@TIHan
Copy link
Contributor Author

TIHan commented Feb 23, 2023

/azp run runtime-coreclr superpmi-diffs

@TIHan
Copy link
Contributor Author

TIHan commented Feb 23, 2023

/azp run runtime-coreclr superpmi-replay

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TIHan
Copy link
Contributor Author

TIHan commented Feb 23, 2023

@dotnet/jit-contrib @jakobbotsch this is ready again. current superpmi replay failure is unrelated

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM!

@TIHan
Copy link
Contributor Author

TIHan commented Feb 23, 2023

Dope

@TIHan TIHan merged commit a375009 into dotnet:main Feb 23, 2023
@TIHan TIHan deleted the x86-skip-and-negative-one branch February 23, 2023 22:09
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants