Skip to content

Comments

Remove redundant ulong casts in Span.Slice for x64 targets#123635

Draft
reedz wants to merge 2 commits intodotnet:mainfrom
reedz:feature/remove-uint-cast-workarounds
Draft

Remove redundant ulong casts in Span.Slice for x64 targets#123635
reedz wants to merge 2 commits intodotnet:mainfrom
reedz:feature/remove-uint-cast-workarounds

Conversation

@reedz
Copy link
Contributor

@reedz reedz commented Jan 26, 2026

Fixes #120607.

Related: #67044 , #119689

Remove 64-bit workarounds (ulong casts for 64-bit arithmetic) to produce JIT-friendly code that eliminates unnecessary bounds checks for Span slices. Presumably, this has become possible thanks to earlier work done in #62864,

Code from the issue:

[MethodImpl(MethodImplOptions.NoInlining)]
static Span<byte> SlicePattern(Span<byte> input)
{
    if (input.Length >= 1)
        return input.Slice(1, input.Length - 1);  // Should have no bounds check
    return input;
}

Before changes:

G_M000_IG01:
            stp     fp, lr, [sp, #-0x10]!
            mov     fp, sp

G_M000_IG02:
            cmp     w1, #0
            bgt     G_M000_IG04

G_M000_IG03:
            ldp     fp, lr, [sp], #0x10
            ret     lr

G_M000_IG04:
            sub     w2, w1, #1          ; compute length - 1
            mov     w3, w2
            add     x3, x3, #1          ; compute 1 + (length - 1)
            cmp     x3, w1, UXTW        ; BOUNDS CHECK: compare with original length
            bhi     G_M000_IG06         ; branch to throw if out of bounds
            add     x0, x0, #1
            mov     x1, x2

G_M000_IG05:
            ldp     fp, lr, [sp], #0x10
            ret     lr

G_M000_IG06:                            ; throw path
            movz    x0, #0xD908
            movk    x0, #0x1012 LSL #16
            movk    x0, #0xEDC5 LSL #32
            ldr     x0, [x0]
            blr     x0
            brk     #0

; Total bytes of code 84

After changes

G_M000_IG01:
            stp     fp, lr, [sp, #-0x10]!
            mov     fp, sp

G_M000_IG02:
            cmp     w1, #0              ; if (input.Length >= 1)
            bgt     G_M000_IG04

G_M000_IG03:
            ldp     fp, lr, [sp], #0x10
            ret     lr

G_M000_IG04:
            sub     w1, w1, #1          ; new length = length - 1
            add     x0, x0, #1          ; new pointer = pointer + 1

G_M000_IG05:
            ldp     fp, lr, [sp], #0x10
            ret     lr

; Total bytes of code 40

Copilot AI review requested due to automatic review settings January 26, 2026 18:33
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 26, 2026
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 26, 2026
Copilot AI reviewed Jan 26, 2026
View reviewed changes
Copy link
Contributor

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 removes platform-specific bounds checking workarounds for 64-bit targets in Span, Memory, and related types. The change simplifies the code by using a uniform bounds check pattern across all platforms, enabling better JIT optimization and eliminating unnecessary bounds checks in common slicing patterns.

Changes:

  • Replaced complex 64-bit-specific bounds checks with simpler, JIT-friendly two-condition checks that work uniformly across all platforms
  • Updated comment in ValueListBuilder.cs to reflect that the guard condition is now universal rather than 64-bit specific
  • Removed all TARGET_64BIT conditional compilation directives related to bounds checking from Span, Memory, String, and related types

Reviewed changes

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

Show a summary per file
File Description
src/libraries/System.Private.CoreLib/src/System/Span.cs Removed TARGET_64BIT conditional for Slice bounds checking in both constructor and Slice method
src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs Removed TARGET_64BIT conditional for Slice bounds checking in both constructor and Slice method
src/libraries/System.Private.CoreLib/src/System/Memory.cs Removed TARGET_64BIT conditional for Slice bounds checking in constructor, Slice method, and Span property getter
src/libraries/System.Private.CoreLib/src/System/ReadOnlyMemory.cs Removed TARGET_64BIT conditional for Slice bounds checking in constructor, Slice method, and Span property getter
src/libraries/System.Private.CoreLib/src/System/String.cs Removed TARGET_64BIT conditional from TryGetSpan method
src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs Removed TARGET_64BIT conditional from Substring method
src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Removed TARGET_64BIT conditionals from AsSpan and AsMemory extension methods
src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs Updated comment to reflect universal application of guard condition and simplified the bounds check

EgorBo
EgorBo reviewed Jan 26, 2026
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs Outdated
int pos = _pos;
Span<T> span = _span;
if ((ulong)(uint)pos + (ulong)(uint)length <= (ulong)(uint)span.Length) // same guard condition as in Span<T>.Slice on 64-bit
if ((uint)pos <= (uint)span.Length - (uint)length) // same guard condition as in Span<T>.Slice
Copy link
Member

@EgorBo EgorBo Jan 26, 2026

Choose a reason for hiding this comment

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

you need the !TARGET_64BIT path from Slice here, this is not the same as before

Copy link
Contributor Author

@reedz reedz Jan 29, 2026

Choose a reason for hiding this comment

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

Thanks, this has been reverted.
@EgorBo is it possible we're seeing multiple regressions because of this change ? Would you mind re-running MihuBot once more for the fresh changes ? It seems I don't have access, and I'm not sure how I can run the same suite locally.

@EgorBo
Copy link
Member

EgorBo commented Jan 26, 2026

Some of my recent JIT changes were preparing for this actually, but I am not yet sure it won't regress today.

#if TARGET_64BIT
            // See comment in Span<T>.Slice for how this works.
            if ((ulong)(uint)start + (ulong)(uint)length > (ulong)(uint)_length)
                ThrowHelper.ThrowArgumentOutOfRangeException();
#else
            if ((uint)start > (uint)_length || (uint)length > (uint)(_length - start))
                ThrowHelper.ThrowArgumentOutOfRangeException();
#endif

the 64bit path messes with JIT's range check analysis which is built mostly around 32-bit integers (because of Array/Span Length being so), but at the same time it's just one condition+branch. The latter produces two jumps and JIT is not smart enough to merge it into one when it can (after opts).

@EgorBo
Copy link
Member

EgorBo commented Jan 26, 2026

@MihuBot

@MihuBot MihuBot mentioned this pull request Jan 26, 2026
Open
@EgorBo
Copy link
Member

EgorBo commented Jan 26, 2026

1512 improved, 2417 regressed

I think it's not ready for merge yet, needs more work in JIT

@jkotas jkotas marked this pull request as draft January 26, 2026 22:08
This was referenced Jan 27, 2026
Open
Open
Open
Open
@build-analysis build-analysis bot mentioned this pull request Jan 27, 2026
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@EgorBo EgorBo EgorBo left review comments

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Slicing a span with a list pattern produces unnecessary bounds check

2 participants

@reedz @EgorBo