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

Could emit branchless form of i >= 0 && j >= 0 for signed integers #61940

Closed
drewnoakes opened this issue Nov 22, 2021 · 6 comments · Fixed by #62689
Closed

Could emit branchless form of i >= 0 && j >= 0 for signed integers #61940

drewnoakes opened this issue Nov 22, 2021 · 6 comments · Fixed by #62689
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI good first issue Issue should be easy to implement, good for first-time contributors
Milestone

Comments

@drewnoakes
Copy link
Member

drewnoakes commented Nov 22, 2021

Given this code:

bool BothZero(int i, int j) => i == 0 && j == 0;

bool BothGTZ(int i, int j) => i >= 0 && j >= 0;

The JIT optimises BothZero with:

    L0000: or edx, ecx
    L0002: sete al
    L0005: movzx eax, al
    L0008: ret

However BothGTZ introduces branching:

    L0000: test ecx, ecx
    L0002: jl short L000c
    L0004: mov eax, edx
    L0006: not eax
    L0008: shr eax, 0x1f
    L000b: ret
    L000c: xor eax, eax
    L000e: ret

The branchless form, equivalent to C# (i | j) >= 0 could have better performance here:

    L0000: or edx, ecx
    L0002: mov eax, edx
    L0004: not eax
    L0006: shr eax, 0x1f
    L0009: ret

category:cq
theme:basic-cq
skill-level:beginner
cost:small
impact:small

@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Nov 22, 2021
@ghost
Copy link

ghost commented Nov 22, 2021

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

Issue Details

Given this code:

bool BothZero(int i, int j) => i == 0 && j == 0;

bool BothGTZ(int i, int j) => i >= 0 && j >= 0;

The JIT optimises BothZero with:

    L0000: or edx, ecx
    L0002: sete al
    L0005: movzx eax, al
    L0008: ret

However BothGTZ introduces branching:

    L0000: test ecx, ecx
    L0002: jl short L000c
    L0004: mov eax, edx
    L0006: not eax
    L0008: shr eax, 0x1f
    L000b: ret
    L000c: xor eax, eax
    L000e: ret

The branchless form, equivalent to C# (i | j) >= 0 could have better performance here:

    L0000: or edx, ecx
    L0002: mov eax, edx
    L0004: not eax
    L0006: shr eax, 0x1f
    L0009: ret
Author: drewnoakes
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@JulieLeeMSFT
Copy link
Member

CC @dotnet/jit-contrib

@EgorBo
Copy link
Member

EgorBo commented Dec 1, 2021

Marking as up-for-grabs as it should not be difficult to alter optOptimizeBools to handle this case.

@EgorBo EgorBo added good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors labels Dec 1, 2021
@EgorBo EgorBo added this to the Future milestone Dec 1, 2021
@EgorBo EgorBo removed the untriaged New issue has not been triaged by the area owner label Dec 1, 2021
@pedrobsaila
Copy link
Contributor

Hi I would like to work on this issue

@AndyAyersMS
Copy link
Member

Hi I would like to work on this issue

Great! I am assigning it to you. If you have questions or need help, just post a note here.

@drewnoakes
Copy link
Member Author

@pedrobsaila you might also like to optimise this similar case:

bool EitherNonZero(int i, int j) => i != 0 || j != 0;

Currently emits:

    L0000: test edx, edx
    L0002: jne short L0012
    L0004: cmp dword ptr [esp+4], 0
    L0009: setne al
    L000c: movzx eax, al
    L000f: ret 4
    L0012: mov eax, 1
    L0017: ret 4

The branchless form, equivalent to C# (i | j) != 0 could have better performance here:

    L0000: or edx, [esp+4]
    L0004: setne al
    L0007: movzx eax, al
    L000a: ret 4

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 12, 2021
@danmoseley danmoseley removed the help wanted [up-for-grabs] Good issue for external contributors label Apr 10, 2022
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Sep 23, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 31, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 30, 2022
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 good first issue Issue should be easy to implement, good for first-time contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants