Skip to content

Remove bounds check for arr1[arr2.Length] pattern #115980

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 26, 2025
Merged

Remove bounds check for arr1[arr2.Length] pattern #115980

merged 1 commit into from
May 26, 2025
+6 −1

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented May 25, 2025 •
edited
Loading

Closes #115793

static bool Repro(string prefix, string path)
{
    if (prefix.Length < path.Length)
        return (path[prefix.Length] == '/');

    return false;
}
 ; Method Program:Repro(System.String,System.String):ubyte (FullOpts)
-      sub      rsp, 40
       mov      ecx, dword ptr [rcx+0x08]
-      mov      r8d, dword ptr [rdx+0x08]
-      cmp      ecx, r8d
+      mov      eax, dword ptr [rdx+0x08]
+      cmp      ecx, eax
       jl       SHORT G_M15110_IG05
       xor      eax, eax
-      add      rsp, 40
       ret      
 G_M15110_IG05:
-      cmp      ecx, r8d
-      jae      SHORT G_M15110_IG07
       mov      eax, ecx
       cmp      word  ptr [rdx+2*rax+0x0C], 47
       sete     al
       movzx    rax, al
-      add      rsp, 40
       ret      
-G_M15110_IG07:
-      call     CORINFO_HELP_RNGCHKFAIL
-      int3     
-; Total bytes of code: 53
+; Total bytes of code: 28

A few diffs

@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 25, 2025
@build-analysis build-analysis bot mentioned this pull request May 26, 2025
Open
3 tasks
@EgorBo EgorBo marked this pull request as ready for review May 26, 2025 09:49
@Copilot Copilot AI review requested due to automatic review settings May 26, 2025 09:49
Copilot
Copilot AI reviewed May 26, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the JIT range-check logic to recognize the new GT_ARR_LENGTH node and remove an unnecessary bounds check for patterns like arr1[arr2.Length], improving codegen size and performance.

  • Treat GT_ARR_LENGTH as non-overflowing in ComputeDoesOverflow
  • Provide a concrete range (0 to CORINFO_Array_MaxLength) for GT_ARR_LENGTH in ComputeRange
  • Eliminate redundant range‐check helper paths, reducing code size
Comments suppressed due to low confidence (1)

src/coreclr/jit/rangecheck.cpp:1647

  • Consider adding unit tests for the GT_ARR_LENGTH case to validate that the computed range (0 to CORINFO_Array_MaxLength) behaves as expected and guard against future regressions in this scenario.
else if (expr->OperIs(GT_ARR_LENGTH))

@EgorBo
Copy link
Member Author

EgorBo commented May 26, 2025

PTAL @jakobbotsch @dotnet/jit-contrib small change, a couple of diffs

@EgorBo EgorBo requested a review from jakobbotsch May 26, 2025 11:04
@EgorBo EgorBo enabled auto-merge (squash) May 26, 2025 17:14
@EgorBo
Copy link
Member Author

EgorBo commented May 26, 2025

/ba-g "NativeAOT timeouts"

@EgorBo EgorBo merged commit ac7f8e0 into dotnet:main May 26, 2025
107 of 111 checks passed
@EgorBo EgorBo deleted the fix-bce-regression branch May 26, 2025 17:15
@rameel rameel mentioned this pull request May 28, 2025
Open
@github-actions github-actions bot locked and limited conversation to collaborators Jun 26, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

Copilot code review Copilot Copilot left review comments

@jakobbotsch jakobbotsch jakobbotsch approved these changes

Assignees

@EgorBo EgorBo

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression: Unnecessary bounds check generated
2 participants
@EgorBo @jakobbotsch