Use safe spans in ReverseEndianness#127411
Conversation
Rewrite the vectorized span helper to use Span slices with Vector*.Create and CopyTo while preserving overlap direction handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-memory |
There was a problem hiding this comment.
Pull request overview
This PR updates the span-based BinaryPrimitives.ReverseEndianness implementation in System.Private.CoreLib to use span slicing plus Vector*.Create(ReadOnlySpan<T>) and CopyTo(Span<T>) for the vectorized path, avoiding explicit ref arithmetic and LoadUnsafe/StoreUnsafe usage while still handling overlapping source/destination spans safely.
Changes:
- Rewrote the generic vectorized
ReverseEndianness<T, TReverser>helper to process via safe span slicing (forward/backward depending on overlap). - Updated overlap-direction checks to rely on
Span.Overlaps(..., out elementOffset)withelementOffset <= 0for forward iteration. - Applied the same overlap-direction simplification to the
Int128overload.
| if (Vector256.IsHardwareAccelerated) | ||
| { | ||
| while (i <= source.Length - Vector256<T>.Count) | ||
| while (source.Length >= Vector256<T>.Count) | ||
| { | ||
| Vector256.StoreUnsafe(TReverser.Reverse(Vector256.LoadUnsafe(ref sourceRef, (uint)i)), ref destRef, (uint)i); | ||
| i += Vector256<T>.Count; | ||
| TReverser.Reverse(Vector256.Create(source)).CopyTo(target); | ||
| source = source.Slice(Vector256<T>.Count); | ||
| target = target.Slice(Vector256<T>.Count); | ||
| } | ||
| } | ||
|
|
||
| if (Vector128.IsHardwareAccelerated) | ||
| { | ||
| while (i <= source.Length - Vector128<T>.Count) | ||
| while (source.Length >= Vector128<T>.Count) | ||
| { | ||
| Vector128.StoreUnsafe(TReverser.Reverse(Vector128.LoadUnsafe(ref sourceRef, (uint)i)), ref destRef, (uint)i); | ||
| i += Vector128<T>.Count; | ||
| TReverser.Reverse(Vector128.Create(source)).CopyTo(target); | ||
| source = source.Slice(Vector128<T>.Count); | ||
| target = target.Slice(Vector128<T>.Count); | ||
| } |
There was a problem hiding this comment.
The Vector256/Vector128 fast paths call Vector*.Create(ReadOnlySpan<T>) and CopyTo(Span<T>), and both APIs perform a destination/source length check internally. Since the surrounding loop already guards source.Length >= Vector*.Count, this can introduce redundant per-iteration checks (unless the JIT reliably inlines and eliminates them), which may regress performance compared to the prior LoadUnsafe/StoreUnsafe approach. Consider switching to a load/store strategy that avoids additional range checks (e.g., read/write unaligned via span references) while keeping the overlap-safe slicing logic.
Note
This PR description was generated by GitHub Copilot.
Summary
Vector*.CreateandCopyToLoadUnsafe/StoreUnsafeusage from this helperValidation
build.cmd clr.corelib+clr.nativecorelib+libs.pretest -rc checkeddotnet.cmd build /t:test src\libraries\System.Memory\tests\System.Memory.Tests.csproj -p:XUnitClassName=System.Buffers.Binary.Tests.ReverseEndiannessUnitTests -v:minimal(368 tests, 0 failed)