Skip to content

JIT: Transform SELECT(relop, 1/0, 0/1) to relop #81880

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

Merged
merged 2 commits into from
Feb 9, 2023

Conversation

jakobbotsch
Copy link
Member

@jakobbotsch jakobbotsch commented Feb 9, 2023

Fix #81479

Initially I generalized this to handle any power of two via LSH(cond, shift). Like GCC and Clang do here: https://godbolt.org/z/hxPf3Ka8e
However, it had worse performance than a cmov from my measurements (around 20% slower). It would use one less register in many situations though.

We could also do this directly in if-conversion, though then we wouldn't get potential future SELECTs created in different places.

Example:

[MethodImpl(MethodImplOptions.NoInlining)]
public bool IfElseCondition(bool input)
{
    if (input)
    {
        return false;
    }
    else
    {
        return true;
    }
}

Before:

G_M21872_IG02:              ;; offset=0000H
       84D2                 test     dl, dl
       7403                 je       SHORT G_M21872_IG05
                                                ;; size=4 bbWeight=1 PerfScore 1.25
G_M21872_IG03:              ;; offset=0004H
       33C0                 xor      eax, eax
                                                ;; size=2 bbWeight=0.50 PerfScore 0.12
G_M21872_IG04:              ;; offset=0006H
       C3                   ret
                                                ;; size=1 bbWeight=0.50 PerfScore 0.50
G_M21872_IG05:              ;; offset=0007H
       B801000000           mov      eax, 1
                                                ;; size=5 bbWeight=0.50 PerfScore 0.12
G_M21872_IG06:              ;; offset=000CH
       C3                   ret
                                                ;; size=1 bbWeight=0.50 PerfScore 0.50

After #81267:

G_M21872_IG02:              ;; offset=0000H
       B801000000           mov      eax, 1
       33C9                 xor      ecx, ecx
       84D2                 test     dl, dl
       0F45C1               cmovne   eax, ecx
                                                ;; size=12 bbWeight=1 PerfScore 1.00
G_M21872_IG03:              ;; offset=000CH
       C3                   ret

After #81267 + this PR:

G_M21872_IG02:              ;; offset=0000H
       33C0                 xor      eax, eax
       84D2                 test     dl, dl
       0F94C0               sete     al
                                                ;; size=7 bbWeight=1 PerfScore 1.50
G_M21872_IG03:              ;; offset=0007H
       C3                   ret

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 9, 2023
@ghost ghost assigned jakobbotsch Feb 9, 2023
@ghost
Copy link

ghost commented Feb 9, 2023

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

Issue Details

Fix #81479

FWIW, initially I generalized this to handle any power of two via LSH(cond, shift). Like GCC and Clang do here: https://godbolt.org/z/hxPf3Ka8e
However, it had worse performance than a cmov from my measurements (around 20% slower). It would use one less register in many situations though.

We could also do this directly in if-conversion, though then we wouldn't get potential future SELECTs created in different places.

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch jakobbotsch changed the title JIT: Transform SELECT(cond, 1/0, 0/1) to cond JIT: Transform SELECT(relop, 1/0, 0/1) to relop Feb 9, 2023
@tannergooding
Copy link
Member

This should help Roslyn/F# for this case: dotnet/roslyn#61483

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Feb 9, 2023

Diffs New diffs

cc @dotnet/jit-contrib PTAL @EgorBo

@jakobbotsch jakobbotsch requested a review from EgorBo February 9, 2023 13:44
if (select->TypeIs(TYP_LONG))
{
GenTree* cast = comp->gtNewCastNode(TYP_LONG, newNode, false, TYP_LONG);
BlockRange().InsertAfter(newNode, cast);
Copy link
Member

Choose a reason for hiding this comment

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

Why cast here is needed? if we know that we return either 0 or 1 - can we leave it TYP_INT (sort of legal after lowering)

Copy link
Member Author

@jakobbotsch jakobbotsch Feb 9, 2023

Choose a reason for hiding this comment

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

Hmm, indeed it looks like compare nodes support being TYP_LONG typed in codegen, so I can just retype it. Will fix

Copy link
Member Author

Choose a reason for hiding this comment

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

New diffs, a bit better now, but I guess it was rare that we needed this cast

@jakobbotsch jakobbotsch merged commit d0fdb17 into dotnet:main Feb 9, 2023
@jakobbotsch jakobbotsch deleted the transform-select-to-setcc branch February 9, 2023 17:30
@cincuranet
Copy link
Contributor

cincuranet commented Feb 16, 2023

@AndyAyersMS
Copy link
Member

@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 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.

Difference of Codegen in JIT ASM when using Negation / if-else block code
5 participants