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

Eliminate bound checks for "arr[index % arr.Length]" #84231

Merged
merged 7 commits into from
Apr 15, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Apr 2, 2023

Closes #9421

Example:

int Bucket(int[] buckets, uint hashcode) => buckets[hashcode % buckets.Length];

Codegen diff:

; Method Bucket(int[],uint):int
-      4883EC28             sub      rsp, 40
       8BC2                 mov      eax, edx
       448B4108             mov      r8d, dword ptr [rcx+08H]
-      4899                 cqo      
-      49F7F8               idiv     rdx:rax, r8
+      33D2                 xor      edx, edx
+      49F7F0               div      rdx:rax, r8
-      493BD0               cmp      rdx, r8
-      7309                 jae      SHORT G_M62171_IG04
       8B449110             mov      eax, dword ptr [rcx+4*rdx+10H]
-      4883C428             add      rsp, 40
       C3                   ret      
-G_M62171_IG04:
-      E8CE62535F           call     CORINFO_HELP_RNGCHKFAIL
-      CC                   int3
-; Total bytes of code: 35
+; Total bytes of code: 35

As was mentioned in the issue, this only works for XARCH since on ARM we always expand a % b early in Morph to a - (a / b) * b. Will be fixed if we move that transformation to lower.

PS: Index has to be something non-negative, e.g. a constant or unsigned type, etc.

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

ghost commented Apr 2, 2023

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

Issue Details

Closes #9421

Example:

int Bucket(int[] buckets, int hashcode) => buckets[hashcode % buckets.Length];

Codegen diff:

; Method Bucket(int[],int):int:this
G_M35976_IG01:              
-      4883EC28             sub      rsp, 40
       488BCA               mov      rcx, rdx
G_M35976_IG02:              
       448B4908             mov      r9d, dword ptr [rcx+08H]
       418BC0               mov      eax, r8d
       99                   cdq      
       41F7F9               idiv     edx:eax, r9d
-      413BD1               cmp      edx, r9d
-      730B                 jae      SHORT G_M35976_IG04
       8BC2                 mov      eax, edx
       8B448110             mov      eax, dword ptr [rcx+4*rax+10H]
G_M35976_IG03:              
-      4883C428             add      rsp, 40
       C3                   ret      
-G_M35976_IG04:
-      E8C962525F           call     CORINFO_HELP_RNGCHKFAIL
-      CC                   int3     
-; Total bytes of code: 40
+; Total bytes of code: 21
Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo
Copy link
Member Author

EgorBo commented Apr 3, 2023

@dotnet/jit-contrib @SingleAccretion @jakobbotsch PTAL - this pattern used to be popular but then we replaced it with Lemire's FastMod in most places, but it's still used here and there (my changes in BCL obviosly didn't make it to SPMI diffs)

The PR does two things:

  1. Recognizes [x UMOD arrlen] in rangecheck
  2. Improves IsNeverNegative function that leads to more cases where we convert X MOD Y to X UMOD Y

@EgorBo
Copy link
Member Author

EgorBo commented Apr 14, 2023

@AndyAyersMS PTAL since you reviewed my previous "eliminate bound checks" PR

@EgorBo EgorBo requested a review from AndyAyersMS April 14, 2023 22:17
@EgorBo
Copy link
Member Author

EgorBo commented Apr 15, 2023

wasm failures are #80619

@EgorBo EgorBo merged commit d0de955 into dotnet:main Apr 15, 2023
@EgorBo EgorBo deleted the mod-length-rangechecks branch April 15, 2023 09:18
@stephentoub
Copy link
Member

There are a fair number of places we use FastMod instead of % to index into an array. The former was strictly better, but now the latter avoids the bounds check the former still incurs. Does this change the equation, or is the former still better enough even with the extra check?

@EgorBo
Copy link
Member Author

EgorBo commented Apr 15, 2023

There are a fair number of places we use FastMod instead of % to index into an array. The former was strictly better, but now the latter avoids the bounds check the former still incurs. Does this change the equation, or is the former still better enough even with the extra check?

I believe that one is still better since it avoids expensive idiv instruction (not that expensive on modern cpus though). E.g. it makes sense to use FastMod even in C++ code - #65926

@stephentoub
Copy link
Member

stephentoub commented Apr 15, 2023

it makes sense to use FastMod even in C++ code

Right, but there neither thing being compared has an extra cmp/branch, whereas now the FastMod one in .NET does but % doesn't.

@EgorBo
Copy link
Member Author

EgorBo commented Apr 15, 2023

it makes sense to use FastMod even in C++ code

Right, but there neither thing being compared has an extra cmp/branch, whereas now the FastMod one in .NET does but % doesn't.

I'd be surprised to see FastMod slower 🙂 div instruction is rougly 20x slower than e.g. Add, on some CPUs it has latency over 100 in some cases. However, it seems to be not that bad on e.g. Apple M1 (SDIV 64bit - Latency=9)

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

[Jit] Indexing mod array.Length not recognized as bounds safe
4 participants