Eliminate bound checks for "% arr.Length" when possible

[henriquewr]

fixes #130155

Pattern [index % arr.Length]:

static int Test(int index, int len)
{
    if (index >= 0)
    {
        var arr = new int[len];

        return arr[index % arr.Length];
    }

    return 1234;
}

went from

; Method BoundCheckTests.Program:Test(int,int):int (FullOpts)
G_M000_IG01:                ;; offset=0x0000
       push     rbx
       sub      rsp, 32
       mov      ebx, ecx

G_M000_IG02:                ;; offset=0x0007
       test     ebx, ebx
       jge      SHORT G_M000_IG05

G_M000_IG03:                ;; offset=0x000B
       mov      eax, 0x4D2

G_M000_IG04:                ;; offset=0x0010
       add      rsp, 32
       pop      rbx
       ret      

G_M000_IG05:                ;; offset=0x0016
       movsxd   rdx, edx
       mov      rcx, 0x7FFD39B6A7D0
       call     CORINFO_HELP_NEWARR_1_VC
       mov      rcx, rax
       mov      r8d, dword ptr [rcx+0x08]
       mov      eax, ebx
       cdq      
       idiv     edx:eax, r8d
       cmp      edx, r8d
       jae      SHORT G_M000_IG07
       mov      eax, edx
       mov      eax, dword ptr [rcx+4*rax+0x10]

G_M000_IG06:                ;; offset=0x0040
       add      rsp, 32
       pop      rbx
       ret      

G_M000_IG07:                ;; offset=0x0046
       call     CORINFO_HELP_RNGCHKFAIL
       int3     
; Total bytes of code: 76

to

; Method BoundCheckTests.Program:Test(int,int):int (FullOpts)
G_M50457_IG01:  ;; offset=0x0000
       push     rbx
       sub      rsp, 32
       mov      ebx, ecx
						;; size=7 bbWeight=1 PerfScore 1.50

G_M50457_IG02:  ;; offset=0x0007
       test     ebx, ebx
       jge      SHORT G_M50457_IG05
						;; size=4 bbWeight=1 PerfScore 1.25

G_M50457_IG03:  ;; offset=0x000B
       mov      eax, 0x4D2
						;; size=5 bbWeight=0.50 PerfScore 0.12

G_M50457_IG04:  ;; offset=0x0010
       add      rsp, 32
       pop      rbx
       ret      
						;; size=6 bbWeight=0.50 PerfScore 0.88

G_M50457_IG05:  ;; offset=0x0016
       movsxd   rdx, edx
       mov      rcx, 0x7FFD2E8773C0      ; int[]
       call     CORINFO_HELP_NEWARR_1_VC
       mov      rcx, rax
       mov      r8d, dword ptr [rcx+0x08]
       mov      eax, ebx
       xor      edx, edx
       div      edx:eax, r8d
       mov      eax, edx
       mov      eax, dword ptr [rcx+4*rax+0x10]
						;; size=38 bbWeight=0.50 PerfScore 15.75

G_M50457_IG06:  ;; offset=0x003C
       add      rsp, 32
       pop      rbx
       ret      
						;; size=6 bbWeight=0.50 PerfScore 0.88
; Total bytes of code: 66

[dotnet-policy-service]

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

[henriquewr]

Is there a way of representing the maximum value possible? (limit is int32 in the ctor)
Im not sure how ranges above 32 bits are representable

[BoyBaykiller]

Rangecheck is 32bit limited.

[EgorBo]

CI failures are related

[henriquewr]

As far as I know adding asserts isn't free
these asserts might be taking the place of a more useful one ((uint)index <= (uint)arr.Length and (uint)arr.Length >= (uint)index)

Is doing this really a good idea?

(Also, I don't really know how often the asserts table gets full)

[EgorBo]

TBH, I am not sure I understand what you're doing, or rather this looks like a typical AI contribution given how AI loves to solve problems b in the jit codebase by introducing new loops over entites vs integrating into existing structures.

I don't think "arr.Length % X" makes any sense in real code, "X % arrLen" does make sense, and it's supported but only for unsigned mods, it seems it should be possible to extend that logic to signed mods as well with the help of assertions, should be a very trivial change, not +300 LOC of very risky changes in an area that has a poor test coverage.

[henriquewr]

loops over entites vs integrating into existing structures.

I could be wrong, but i think you are talking about the (ugly) while loops, as far as I searched in the codebase, thats the only way of knowing if an assert is know to be applicable

I don't think "arr.Length % X" makes any sense in real code

Agree, i dont really think thats the mosty useful thing, but since signed operators already has all the assertions needed (<=, >=, >, <) I just implemented anyway

should be a very trivial change, not +300 LOC

yeah, most of the code is the ugly loop for assertions to support the not so useful length % index pattern (removing the support for this would remove around 120 loc, including the risky modifications on optCreateJTrueBoundsAssertion)

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "1fcf6f239d9cfcff26376f931ebf20574204020c",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "d03f6b9564cc85413a7cfaa000faeabb82abea47",
  "last_reviewed_commit": "1fcf6f239d9cfcff26376f931ebf20574204020c",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "d03f6b9564cc85413a7cfaa000faeabb82abea47",
  "last_recorded_worker_run_id": "29695950482",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "c9fea9f8fec1cc3f0d7812ba6af9d96d01fe58bb",
      "review_id": 4730676952
    },
    {
      "commit": "1fcf6f239d9cfcff26376f931ebf20574204020c",
      "review_id": 4731159221
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Fixes #130155. The JIT already eliminates the array bounds check for the arr[X u% arr.Length] (unsigned remainder) pattern, since an unsigned remainder by the length is always in [0, arr.Length). This PR extends the same optimization to the signed remainder pattern arr[X % arr.Length] when the index X is provably non-negative, which is a very common idiom (e.g. after an if (index >= 0) guard). The generated code drops the redundant CORINFO_HELP_RNGCHKFAIL path.

Approach: Three coordinated changes: (1) optAssertionProp_BndsChk gains an else if branch matching VNF_MOD(X, arr.Length); it computes the range of X and drops the check when the lower limit is a known constant >= 0. This is safe because for non-negative X, X % len lies in [0, len), and the len == 0 case still throws DivideByZeroException before any access. (2) VNF_MOD is added to the range-propagation switch in RangeCheck::GetRangeFromAssertionsWorker, dispatching to a new RangeOps::Mod. (3) RangeOps::Mod computes a conservative signed-remainder range, handling single-value constants, constant divisor ranges (using abs(divisor)-1 with INT32_MIN guarding), and sign-based bounds for the dividend. The test file adds signed/unsigned variants covering >, >=, <, <= guards and DivideByZeroException cases. The run.py change makes XML time parsing tolerant of comma decimal separators produced under non-invariant cultures.

Summary: The optimization is well-scoped and mirrors the existing, proven VNF_UMOD handling, with the crucial added guard that the dividend is known non-negative — necessary because signed % can yield negative results for negative dividends. The RangeOps::Mod implementation is conservative and the INT32_MIN overflow in abs() is explicitly guarded via SafeAbsMinusOne. The added tests exercise the relevant guard shapes and the div-by-zero behavior. Note that per the commit history the more speculative arr.Length % index support was removed, narrowing the change to the safe, common case. Only a minor cosmetic nit (stray indentation on the [Fact] attribute) was found; no correctness concerns. Verdict: LGTM. Note this worker cannot run builds/tests; test behavior was assessed by reading, and CI status should be consulted for the earlier flagged failures.

Detailed Findings

• src/tests/JIT/opt/RangeChecks/ModLength.cs (line 10): cosmetic — stray extra indentation was introduced on the [Fact] attribute. Flagged inline.

No actionable correctness or performance issues were identified in the changed lines. The -0 literals in RangeOps::Mod (e.g. rightLimit = ... ? -0 : ...) are equivalent to 0 for int and harmless, but reading as 0 would be marginally clearer.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 134.7 AIC · ⌖ 10.7 AIC · ⊞ 10K

[github-actions]

Holistic Review

Motivation: Fixes #130155. Extends the JIT bounds-check elimination that already applies to the unsigned remainder pattern arr[X u% arr.Length] to the signed remainder pattern arr[X % arr.Length] when the index X is provably non-negative — a common idiom after an if (index >= 0) guard — dropping the redundant CORINFO_HELP_RNGCHKFAIL path.

Approach: Three coordinated changes: (1) optAssertionProp_BndsChk gains an else if branch matching VNF_MOD(X, arr.Length), computing the range of X and dropping the check when its lower limit is a known constant >= 0 (safe because for non-negative X, X % len lies in [0, len), and len == 0 still throws DivideByZeroException before any access). (2) VNF_MOD is added to the range-propagation switch, dispatching to a new RangeOps::Mod. (3) RangeOps::Mod computes a conservative signed-remainder range with INT32_MIN guarded via SafeAbsMinusOne. Tests cover signed/unsigned variants across >, >=, <, <= guards and div-by-zero cases; a run.py change makes XML time parsing tolerant of comma decimal separators. The single incremental commit since the prior review (1fcf6f2, "Wrong identation") fixes the stray [Fact] indentation flagged previously.

Summary: The incremental change corrects the one cosmetic nit identified in the prior review, restoring correct indentation on the [Fact] attribute in ModLength.cs. No functional code changed since the last review; the optimization remains well-scoped, mirrors the proven VNF_UMOD handling, and carries the crucial non-negative-dividend guard. No new correctness or performance concerns in the incremental scope. Verdict: LGTM. This worker cannot run builds/tests; test behavior was assessed by reading and CI status should be consulted.

Assessment History:

• review 4730676952 (commit c9fea9f): verdict LGTM with one cosmetic indentation nit. Current verdict: LGTM. Changed — the sole flagged nit is now resolved by commit 1fcf6f2; motivation, approach, and risk are otherwise unchanged.

Detailed Findings

No actionable findings in the incremental scope. The prior cosmetic indentation nit on the [Fact] attribute has been fixed.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 40.6 AIC · ⌖ 9.85 AIC · ⊞ 10K