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

Fold "X >= 0 && X < NN" to "X u< NN" #93834

Merged
merged 2 commits into from
Oct 27, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Oct 22, 2023

Fold e.g. X >= 0 && X < arr.Length to (uint)X < arr.Length. Not too many diffs because:

  1. We already use uint hack (I call it a hack because IMO it's not how most developers check ranges. Also, it's not "use checked context" friendly) everywhere in BCL
  2. For arrays we need to land Add GT_ARR_LEN to optNonNullAssertionProp_Ind #93531 first (and that depends on a fix for a CSE issue)
static void Test(Span<int> span, int i)
{
    if (i >= 0 && i < span.Length)
        Console.WriteLine("In range!");
}
; Method InitblkPerformanceIssue.Program:Test(System.Span`1[int],int) (FullOpts)
       sub      rsp, 40
-      test     edx, edx
-      jl       SHORT G_M55087_IG04
       cmp      edx, dword ptr [rcx+0x08]
-      jge      SHORT G_M55087_IG04
+      jae      SHORT G_M55087_IG04
       mov      rcx, 0x1AC80209320      ; 'In range!'
       call     [System.Console:WriteLine(System.String)]
G_M55087_IG04:
       nop      
       add      rsp, 40
       ret      
-; Total bytes of code: 35
+; Total bytes of code: 31

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

ghost commented Oct 22, 2023

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

Issue Details

Fold e.g. X >= 0 && X < arr.Length to (uint)X < arr.Length. Not too many diffs because:

  1. We already use uint hack (I call it a hack because IMO it's now how most developers check ranges. Also, it's not "use checked context" friendly) everywhere in BCL
  2. For arrays we need to land Add GT_ARR_LEN to optNonNullAssertionProp_Ind #93531 first (and that depends on a fix for a CSE issue)
static void Test(Span<int> span, int i)
{
    if (i >= 0 && i < span.Length)
        Console.WriteLine("In range!");
}
; Method InitblkPerformanceIssue.Program:Test(System.Span`1[int],int) (FullOpts)
       sub      rsp, 40
-      test     edx, edx
-      jl       SHORT G_M55087_IG04
       cmp      edx, dword ptr [rcx+0x08]
-      jge      SHORT G_M55087_IG04
+      jae      SHORT G_M55087_IG04
       mov      rcx, 0x1AC80209320      ; 'In range!'
       call     [System.Console:WriteLine(System.String)]
G_M55087_IG04:
       nop      
       add      rsp, 40
       ret      
-; Total bytes of code: 35
+; Total bytes of code: 31
Author: EgorBo
Assignees: EgorBo
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo marked this pull request as ready for review October 23, 2023 00:37
@AndyAyersMS
Copy link
Member

Would still prefer we sort these out earlier, but also like having some "defense in depth."

  1. We already use uint hack (I call it a hack because IMO it's not how most developers check ranges. Also, it's not "use checked context" friendly) everywhere in BCL

Once take this change, can we undo some of these?

@kunalspathak kunalspathak merged commit 910fb04 into dotnet:main Oct 27, 2023
139 checks passed
@markples
Copy link
Member

@EgorBo just a few notes on the test as I looked through it due to #94146. Some are just ideas to make test authoring easier in the future and might not be worth retroactive changes

  • The messages in the test exceptions have copy/paste errors. I believe that TestEntryPoint could be split into 3 (or 4) different [Fact] methods and then the messages wouldn't be needed at all as the test names would suffice.
  • I think Assert.Throws could be used instead of reimplementing it.
  • Are Test_Span1 and Test_Array identical? I don't understand the comment in Test_Array about arr being null since arr is a Span.

@BruceForstall
Copy link
Member

Please cc @dotnet/jit-contrib for JIT PRs.

@EgorBo
Copy link
Member Author

EgorBo commented Oct 31, 2023

Please cc @dotnet/jit-contrib for JIT PRs.

It wasn't merged by me, I planned to ask for review once I am back from vacation 🤷‍♂️ It's unfortunate that return 100; typo in the test didn't make CI red.

@markples thanks, will apply the suggestions as part of some other PR

@EgorBo EgorBo deleted the fold-unsigned-cmp2 branch October 31, 2023 17:14
@kunalspathak
Copy link
Member

It wasn't merged by me, I planned to ask for review once I am back from vacation

Sorry, i was going through the "approved PRs" list and merged this one.

It's unfortunate that return 100; typo in the test didn't make CI red.

Do we know why this happened and if there is a fix needed?

@EgorBo
Copy link
Member Author

EgorBo commented Oct 31, 2023

Do we know why this happened and if there is a fix needed?

#94146 (comment) it's not clear yet

liveans pushed a commit to liveans/dotnet-runtime that referenced this pull request Nov 9, 2023
* Fold "X >= 0 && X < NN" to "X u< NN"

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

5 participants