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: Remove bound checks when index is Byte and array.Length >= 256 #25912

Open
wants to merge 7 commits into
base: master
from

Conversation

@EgorBo
Copy link
Contributor

commented Jul 27, 2019

Fixes #25894
Test case:

static ReadOnlySpan<byte> _byteArray => new byte[256]
{
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,
    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0
};

static byte GetByte(int index)
{
    return _byteArray[(byte)index]; // System.Byte.MaxValue <= map.Length
                                    // so index will never go out of bounds
}

Was:

; Method Program:GetByte(int):ubyte
G_M30997_IG01:
       sub      rsp, 40

G_M30997_IG02:
       movzx    rax, cl
       cmp      eax, 256
       jae      SHORT G_M30997_IG04
       movsxd   rax, eax
       mov      rdx, 0xD1FFAB1E
       movzx    rax, byte  ptr [rax+rdx]

G_M30997_IG03:
       add      rsp, 40
       ret      

G_M30997_IG04:
       call     CORINFO_HELP_RNGCHKFAIL
       int3     

; Total bytes of code 42

Now:

; Method Program:GetByte(int):ubyte

G_M30997_IG01:

G_M30997_IG02:
       movzx    rax, cl
       movsxd   rax, eax
       mov      rdx, 0xD1FFAB1E
       movzx    rax, byte  ptr [rax+rdx]

G_M30997_IG03:
       ret      
; Total bytes of code: 21

jit-diff:

Summary:
(Lower is better)
Total bytes of diff: -361 (-0.01% of base)
    diff is an improvement.
Top file improvements by size (bytes):
        -361 : System.Private.CoreLib.dasm (-0.01% of base)
1 total files with size differences (1 improved, 0 regressed), 0 unchanged.
Top method improvements by size (bytes):
         -22 (-9.73% of base) : System.Private.CoreLib.dasm - Char:IsNumber(ref,int):bool
         -22 (-6.25% of base) : System.Private.CoreLib.dasm - CultureInfo:VerifyCultureName(ref,bool):bool (2 methods)
         -21 (-10.99% of base) : System.Private.CoreLib.dasm - Char:IsPunctuation(ref,int):bool
         -21 (-10.99% of base) : System.Private.CoreLib.dasm - Char:IsSymbol(ref,int):bool
         -18 (-40.91% of base) : System.Private.CoreLib.dasm - Char:GetLatin1UnicodeCategory(ushort):int
Top method improvements by size (percentage):
         -18 (-40.91% of base) : System.Private.CoreLib.dasm - Char:GetLatin1UnicodeCategory(ushort):int
         -14 (-21.54% of base) : System.Private.CoreLib.dasm - Rune:GetUnicodeCategory(struct):int
         -14 (-20.59% of base) : System.Private.CoreLib.dasm - Char:GetUnicodeCategory(ushort):int
         -14 (-17.72% of base) : System.Private.CoreLib.dasm - Char:IsControl(ushort):bool
         -14 (-15.73% of base) : System.Private.CoreLib.dasm - Rune:IsLetterOrDigit(struct):bool
23 total methods with size differences (23 improved, 0 regressed), 18907 unchanged.
Completed analysis in 2.76s

There are more candidates for this improvement.

PS: I know I have a couple of unfinished PRs, going to focus on them now.

EgorBo added some commits Jul 27, 2019

src/jit/rangecheck.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.