Skip to content

Remove duplicated SVE GatherVector APIs#124033

Open
ylpoonlg wants to merge 3 commits intodotnet:mainfrom
ylpoonlg:github-fix_sve_gather_api
Open

Remove duplicated SVE GatherVector APIs#124033
ylpoonlg wants to merge 3 commits intodotnet:mainfrom
ylpoonlg:github-fix_sve_gather_api

Conversation

@ylpoonlg
Copy link
Contributor

@ylpoonlg ylpoonlg commented Feb 5, 2026

These APIs are duplicated with the normal GatherVector methods, as 32-bit values doesn't need to be extended to 32-bit.

See #103370, which removed the Int32 extensions, but did not remove the UInt32 extensions.

@dotnet/arm64-contrib @a74nh

These are duplicated with the normal GatherVector methods, as 32-bit
values doesn't need to be extended to 32-bit.

See dotnet#103370, which removed the Int32 extensions, but did not remove the
UInt32 extensions.
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 5, 2026
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

@tannergooding tannergooding enabled auto-merge (squash) February 5, 2026 16:52
@a74nh
Copy link
Contributor

a74nh commented Feb 9, 2026

Given this is removing released APIs, I think this might need a API break issue raised against it?

@tannergooding
Copy link
Member

Given this is removing released APIs, I think this might need a API break issue raised against it?

All the APIs are [Experimental] so they aren't "released" and we don't strictly need a breaking change doc.

I don't have a preference if we want to have a general callout for all the fixes we've made in .NET 11 to the experimental surface or not, will leave that up to everyone else.

@stephentoub
Copy link
Member

Code Review: PR #124033 - Remove duplicated SVE GatherVector APIs

Holistic Assessment

Motivation: This PR removes duplicated SVE GatherVectorUInt32 API overloads. The removed APIs that operate on 32-bit values (Vector/Vector) are semantically equivalent to the regular GatherVector methods since 32-bit values don't need to be "zero-extended to 32-bit" - they're already 32-bit. PR #103370 previously removed the Int32 versions, but the UInt32 versions were missed.

Approach: The PR:

  1. Removes redundant API overloads from Sve.cs and Sve.PlatformNotSupported.cs
  2. Updates the reference assembly (System.Runtime.Intrinsics.cs)
  3. Updates the JIT intrinsic table (hwintrinsiclistarm64sve.h) to only enable instructions for 64-bit types
  4. Adds API compatibility suppressions for the breaking change (CP0002)
  5. Removes corresponding test cases

Net positive: ✅ Yes - this is a necessary API cleanup to remove semantically incorrect/duplicate APIs, following the precedent set in #103370.


Detailed Findings

✅ API Removals (Sve.cs, Sve.PlatformNotSupported.cs, System.Runtime.Intrinsics.cs)

  • Correctly removes 16 overloads (4 methods × 4 overloads each) with Vector<int> or Vector<uint> as the return type
  • Retains the 64-bit variants (Vector<long>, Vector<ulong>) which are semantically correct
  • Parallel removals in both the implementation and PlatformNotSupported stub files

✅ JIT Intrinsic Table Changes (hwintrinsiclistarm64sve.h)

  • Changes INS_sve_ld1w and INS_sve_ldff1w entries from supporting int/uint positions to INS_invalid
  • This correctly restricts the 4 affected intrinsics to only long/ulong types

✅ API Compatibility Baseline (ApiCompatBaseline.NetCoreAppLatestStable.xml)

  • All 16 removed API suppressions are correctly added with CP0002 diagnostic IDs
  • Left/Right versions (net10.0 → net11.0) are correct for this .NET 11 timeframe

✅ Test Removals (SveTests.cs)

  • Removes test cases for the removed APIs, keeping tests for the retained 64-bit variants

Summary

Verdict: ✅ Approve

This PR is a clean follow-up to #103370 that removes the remaining incorrectly-designed UInt32 "zero-extend" gather APIs. The change is breaking but correctly suppressed via the API compatibility baseline. All files are consistently updated and the approach matches the prior art.

No issues found.

@stephentoub
Copy link
Member

🤖 Copilot Code Review — PR #124033

Holistic Assessment

Motivation: This PR removes 16 semantically incorrect API overloads that were duplicates of regular GatherVector methods. The removed APIs claim to "zero-extend 32-bit to 64-bit" but return Vector<int> or Vector<uint> — a 32-bit value cannot be meaningfully "zero-extended to 32-bit." PR #103370 previously removed the Int32-returning variants; this PR completes the cleanup by removing the UInt32-returning variants.

Approach: The PR correctly:

  1. Removes the 16 redundant overloads from Sve.cs and Sve.PlatformNotSupported.cs
  2. Updates the ref assembly to match
  3. Restricts JIT intrinsic table entries to only support 64-bit types (INS_invalid for int/uint slots)
  4. Adds API compatibility suppressions (CP0002) for the breaking change
  5. Removes corresponding test data

Summary: ✅ LGTM — This is a clean API cleanup that follows established precedent from #103370. All file changes are consistent, the breaking change is correctly documented in the API compat baseline, and the APIs being removed are experimental ([Experimental]).


Detailed Findings

✅ API Removals — Correct and Complete

The 16 removed overloads fall into 4 methods × 4 overloads each:

  • GatherVectorUInt32WithByteOffsetsZeroExtend (returning Vector<int> with Vector<int> or Vector<uint> offsets, and Vector<uint> versions)
  • GatherVectorUInt32WithByteOffsetsZeroExtendFirstFaulting (same pattern)
  • GatherVectorUInt32ZeroExtend (same pattern)
  • GatherVectorUInt32ZeroExtendFirstFaulting (same pattern)

The 64-bit variants (Vector<long>, Vector<ulong>) are correctly retained — these make semantic sense for "zero-extend from 32-bit to 64-bit."

✅ JIT Intrinsic Table — Correctly Narrowed

The changes to hwintrinsiclistarm64sve.h correctly mark the int/uint type slots (positions 4-5 in the instruction array) as INS_invalid, leaving only long/ulong (positions 6-7) with valid instructions.

✅ Breaking Change Handling — Appropriate

All 16 suppressions are correctly added with:

  • DiagnosticId: CP0002 (member exists in left but not right)
  • Left: net10.0/System.Runtime.Intrinsics.dll
  • Right: net11.0/System.Runtime.Intrinsics.dll

Since these APIs are marked [Experimental], formal breaking change documentation is not strictly required per maintainer feedback.

✅ Test Updates — Consistent

Test data for the removed overloads has been removed from SveTests.cs.


Multi-model review: GPT-5 concurred with LGTM verdict.

@dhartglassMSFT
Copy link
Contributor

jit pieces lgtm
If it's unclear, I think the Unexpected HW intrinsic failures from spmi are expected when removing an intrinsic like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Runtime.Intrinsics community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments