JIT: Elide transitive array bounds checks

[EgorBo]

Closes #121795

static int Test(int[] items, int size, int index)
{
    if ((uint)size <= (uint)items.Length && (uint)index < (uint)size)
    {
        return items[index];
    }
    return 0;
}

 G_M24430_IG01:
-       sub      rsp, 40
 
 G_M24430_IG06:
-       cmp      r8d, r10d
-       jae      SHORT G_M24430_IG08
        mov      eax, r8d
        mov      eax, dword ptr [rcx+4*rax+0x10]
 
-G_M24430_IG08:
-       call     CORINFO_HELP_RNGCHKFAIL
-       int3
-
-; Total bytes of code: 48
+; Total bytes of code: 23

Diffs

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR extends JIT assertion propagation + range analysis so it can chain certain unsigned relations involving checked-bound VNs (e.g., size u<= items.Length) to prove a subsequent index relation (e.g., index u< size) and thereby elide the transitive array bounds check for items[index].

Changes:

• Teach optCreateJTrueBoundsAssertion to create/selectively admit unsigned VN-to-VN and checked-bound assertions that can participate in chaining.
• Update RangeCheck::MergeEdgeAssertionsWorker to (a) accept LE_UN checked-bound assertions and (b) derive bounds through transitive unsigned VN relations while preserving checked-bound-shaped limits.
• Add a JIT opt test covering the “index < size <= length” pattern and asserting no CORINFO_HELP_RNGCHKFAIL appears in the generated code.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/tests/JIT/opt/RangeChecks/ElidedBoundsChecks.cs	Adds a new regression/opt-pattern method and invocation to validate transitive unsigned bounds-check elimination.
src/coreclr/jit/rangecheck.cpp	Enables transitive reasoning across unsigned relations (with guardrails) and preserves checked-bound limits when following VN chains.
src/coreclr/jit/assertionprop.cpp	Broadens assertion creation to support the unsigned checked-bound chaining needed by the rangecheck optimization, with heuristics to limit table pressure.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "e717cbe13c8ecc43769e4e1f4d189b0c68296164",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "92bc4faf0dbeb11fd12efd1d033df8990de73095",
  "last_reviewed_commit": "e717cbe13c8ecc43769e4e1f4d189b0c68296164",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "92bc4faf0dbeb11fd12efd1d033df8990de73095",
  "last_recorded_worker_run_id": "29686414351",
  "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": "e717cbe13c8ecc43769e4e1f4d189b0c68296164",
      "review_id": 4730740563
    }
  ]
}

[github-actions]

Holistic Review

Motivation: The JIT fails to eliminate a redundant bounds check in the transitive pattern if ((uint)size <= (uint)items.Length && (uint)index < (uint)size) items[index]. Even though index < size <= items.Length proves index is in range, RangeCheck could not chain the two unsigned comparisons through the intermediate VN size, so a CORINFO_HELP_RNGCHKFAIL check (and its stack frame) survived. This closes #121795.

Approach: The PR teaches value numbering to flag not just bounds-check length operands but also index operands (m_checkedBoundIndexVNs, SetVNIsCheckedBound(vn, isIndex), IsVNCheckedBoundIndex). In optCreateJTrueBoundsAssertion two new producers are added: (1) unsigned relops against an IsVNArrLen operand now emit a CompareCheckedBound assertion with isVNNeverNegative=true (so size u<= items.Length becomes a recognizable OAK_LE_UN fact), and (2) index u< X where index is a checked-bound index and X already has assertions emits a symbolic CreateRelopVN (OAK_LT_UN). RangeCheck's MergeEdgeAssertionsWorker gains two consumers: an OAK_LE_UN handler that maps size u<= Length to the inclusive keBinOpArray(Length, 0) limit, and an OAK_LT_UN handler that, for index u< size, recursively derives size's range against the preferred bound and, when size in [const>=0, Length + c<=0], tightens index to < Length.

Summary: The change is careful and the reasoning is sound. The unsigned-comparison chaining correctly preserves the non-negativity requirements: the LE_UN consumer sets lLimit = 0 (valid because size u<= Length with Length never-negative implies size is a non-negative int32), and the recursive LT_UN consumer explicitly requires the intermediate bound's lower limit to be a constant >= 0 and its upper limit to be keBinOpArray(preferredBoundVN, cns) with cns <= 0 before concluding index < Length -- correctly rejecting the unsound cns > 0 case. Recursion is bounded (initial budget 3, decremented per level, gated on budget > 0), and the new LT_UN branch is guarded on a fully-unknown pRange so it only fires as a fallback. The isVNNeverNegative propagation on CreateCompareCheckedBound is a safe additive-OR of the pre-existing computed value.

One non-blocking observation: per the JIT area guidance, targeted regression tests are encouraged for changes with a clear repro and observable codegen change. This PR has both (the exact Test repro in the description eliminates a bounds check), yet no JIT regression test is added. A small JitBlue-style test asserting the transitive pattern optimizes (and that near-miss shapes remain correct) would lock in the behavior and guard the new symbolic-VN chaining against future regressions. Correctness itself is well covered by SuperPMI/differential testing, so this is a suggestion rather than a blocker.

Verdict: LGTM, with a suggestion to add a targeted regression test.

Note

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

Generated by Holistic Review · 148.8 AIC · ⌖ 14.7 AIC · ⊞ 10K