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: Optimize constant range tests #93521

Merged
merged 8 commits into from
Oct 17, 2023
Merged

JIT: Optimize constant range tests #93521

merged 8 commits into from
Oct 17, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Oct 14, 2023

void Test(char c)
{
    if ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z'))
        Console.WriteLine("a-zA-Z");

// also: "X < CNS1 || X > CNS2", etc
}

Current codegen:

; Assembly listing for method Prog:Test(ushort):this (FullOpts)
       movzx    rcx, dx
       cmp      ecx, 97
       jl       SHORT G_M56518_IG04
       cmp      ecx, 122
       jle      SHORT G_M56518_IG05
G_M56518_IG04:
       cmp      ecx, 65
       jl       SHORT G_M56518_IG07
       cmp      ecx, 90
       jg       SHORT G_M56518_IG07
G_M56518_IG05:
       mov      rcx, 0x13A00208D40      ; 'a-zA-Z'
       tail.jmp [System.Console:WriteLine(System.String)]
G_M56518_IG07:
       ret      
; Total bytes of code 40

New codegen:

; Assembly listing for method Prog:Test(ushort):this (FullOpts)
       movzx    rcx, dx
       mov      eax, ecx
       sub      eax, 97
       cmp      eax, 25
       jbe      SHORT G_M56518_IG04
       sub      ecx, 65
       cmp      ecx, 25
       ja       SHORT G_M56518_IG06
G_M56518_IG04:
       mov      rcx, 0x1E200008D40      ; 'a-zA-Z'
G_M56518_IG05:
       tail.jmp [System.Console:WriteLine(System.String)]
G_M56518_IG06:
       ret      
; Total bytes of code 38

Not a big size win (sometimes even a size regression actually), but less branches (takes PGO into account).

Alternative patterns:

  • c < 'a' || c > 'z'
  • c is >= 'a' and <= 'z' or >= 'A' and <= 'Z'

@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 Oct 14, 2023
@ghost ghost assigned EgorBo Oct 14, 2023
@ghost
Copy link

ghost commented Oct 14, 2023

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

Issue Details
void Test(char c)
{
    if ((c >= 'a' && c <= 'z') || 
        (c >= 'A' && c <= 'Z'))
        Console.WriteLine("a-zA-Z");
}

Current codegen:

; Assembly listing for method Prog:Test(ushort):this (FullOpts)
       movzx    rcx, dx
       cmp      ecx, 97
       jl       SHORT G_M56518_IG04
       cmp      ecx, 122
       jle      SHORT G_M56518_IG05
G_M56518_IG04:
       cmp      ecx, 65
       jl       SHORT G_M56518_IG07
       cmp      ecx, 90
       jg       SHORT G_M56518_IG07
G_M56518_IG05:
       mov      rcx, 0x13A00208D40      ; 'a-zA-Z'
       tail.jmp [System.Console:WriteLine(System.String)]
G_M56518_IG07:
       ret      
; Total bytes of code 40

New codegen:

; Assembly listing for method Prog:Test(ushort):this (FullOpts)
       movzx    rcx, dx
       mov      eax, ecx
       sub      eax, 97
       cmp      eax, 25
       jbe      SHORT G_M56518_IG04
       sub      ecx, 65
       cmp      ecx, 25
       ja       SHORT G_M56518_IG06
G_M56518_IG04:
       mov      rcx, 0x1E200008D40      ; 'a-zA-Z'
G_M56518_IG05:
       tail.jmp [System.Console:WriteLine(System.String)]
G_M56518_IG06:
       ret      
; Total bytes of code 38

Not a big size win (sometimes even a size regression actually), but less branches (takes PGO into account).

Alternative patterns:

  • c < 'a' || c > 'z'
  • c is >= 'a' and <= 'z'
Author: EgorBo
Assignees: EgorBo
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo changed the title JIT: Optimize range tests JIT: Optimize constant range tests Oct 14, 2023
@EgorBo
Copy link
Member Author

EgorBo commented Oct 14, 2023

/azp list

@azure-pipelines

This comment was marked as resolved.

@EgorBo
Copy link
Member Author

EgorBo commented Oct 14, 2023

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@EgorBo EgorBo marked this pull request as ready for review October 15, 2023 09:36
@EgorBo
Copy link
Member Author

EgorBo commented Oct 16, 2023

@AndyAyersMS @jakobbotsch @dotnet/jit-contrib PTAL, diffs are not too big because we only save 2 bytes on average, but we get rid of a branch, so, hopefully, it will be noticeable. Outerloop/Jitstress/fuzzlyn pass. Can be separately extended to:

  • if (x >= 0 && x < arr.Length) -> if ((uint)x < arr.Length) (if arr is not null)
  • if (x > 10 && x > 100) -> if (x > 100) (although, for this one it's better to be done in RedundantBranchOpts)

Motivated by https://github.com/ricomariani/wrath-othello benchmark.

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.

Nice.

I think it would be a bit more challenging to fit this sort of thing into RBO because it modifies the upper branch and not the lower one. But still worth doing someday.

src/coreclr/jit/optimizebools.cpp Outdated Show resolved Hide resolved
@EgorBo
Copy link
Member Author

EgorBo commented Oct 17, 2023

Failure is #93527

@EgorBo EgorBo merged commit 9279738 into dotnet:main Oct 17, 2023
126 of 129 checks passed
@EgorBo EgorBo deleted the opt-rangetests branch October 17, 2023 11:17
@rhuijben
Copy link

rhuijben commented Oct 18, 2023

I assume it isn't worth the effort to detect+apply the bit fiddling to map lower to upper case (masking 1 bit) and then check a single range, like in the new ascii helpers?
That would also remove the branch, but at another level.

@EgorBo
Copy link
Member Author

EgorBo commented Oct 18, 2023

I assume it isn't worth the effort to detect+apply the bit fiddling to map lower to upper case (masking 1 bit) and then check a single range, like in the new ascii helpers? That would also remove the branch, but at another level.

I am not sure I understood, can you explaind it via code? 🙂

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

3 participants