Skip to content

Sve: Expand CreateWhile APIs#124081

Open
a74nh wants to merge 1 commit intodotnet:mainfrom
a74nh:maskfloat_while_github
Open

Sve: Expand CreateWhile APIs#124081
a74nh wants to merge 1 commit intodotnet:mainfrom
a74nh:maskfloat_while_github

Conversation

@a74nh
Copy link
Contributor

@a74nh a74nh commented Feb 6, 2026

Fixes #108233
Follows on from #123947

*Rename CreateWhile APIs to make it clear they are Unsigned
*Add Signed variants
*Add Double and Single variants

Copilot AI review requested due to automatic review settings February 6, 2026 10:45
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 6, 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.

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Fixes dotnet#108233
Follows on from dotnet#123947

*Rename CreateWhile APIs to make it clear they are Unsigned
*Add Signed variants
*Add Double and Single variants
Copilot AI review requested due to automatic review settings February 6, 2026 13:58
@a74nh a74nh force-pushed the maskfloat_while_github branch from 92469e8 to 4cef667 Compare February 6, 2026 13:58
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

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

@a74nh a74nh requested a review from Copilot February 6, 2026 15:34
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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@a74nh
Copy link
Contributor Author

a74nh commented Feb 9, 2026

@dotnet/arm64-contrib @tannergooding

@stephentoub
Copy link
Member

Code Review: PR #124081 - Sve: Expand CreateWhile APIs

📋 Holistic Assessment

Motivation: This PR addresses issue #108233 and follows on from PR #123947. The change:

  1. Renames existing CreateWhileLessThan* and CreateWhileLessThanOrEqual* APIs from bit-width naming (e.g., CreateWhileLessThanMask8Bit) to type-specific naming (e.g., CreateWhileLessThanMaskByte, CreateWhileLessThanMaskSByte)
  2. Adds signed integer variants (Int16, Int32, Int64, SByte) alongside unsigned variants
  3. Adds floating-point variants (Single, Double)

Approach: The implementation is thorough and consistent across all layers:

  • JIT intrinsic definitions and codegen
  • Managed API surface (Sve.cs and Sve.PlatformNotSupported.cs)
  • Reference assembly (System.Runtime.Intrinsics.cs)
  • API compatibility baseline suppressions
  • Test infrastructure and regression test updates

Net Positive: ✅ Yes - the new naming convention is more intuitive and aligns with the general .NET naming pattern (type-suffixed intrinsics like CreateTrueMaskByte, CreateTrueMaskDouble, etc.).


✅ Positive Findings

Category Details
Consistency Naming now matches other SVE APIs like CreateTrueMask* which already use type suffixes
API Design Adding signed and floating-point variants makes the API surface complete and allows masks to be used with the appropriate vector element types
Breaking Change Handling Properly documented in ApiCompatBaseline.NetCoreAppLatestStable.xml with CP0002 suppressions for removed APIs
Test Coverage Test generator updated with comprehensive tests for all new variants including signed types (SByte, Int16, Int32, Int64) and floating-point types (Single, Double)
Regression Tests Existing tests in Runtime_118377.cs, ChangeMaskUse.cs, and MaskUse.cs updated to use new API names
Documentation XML documentation with proper ARM ACLE references maintained

💡 Observations (Informational)

Category Details
Return Type Change The original CreateWhileLessThanMask16Bit returned Vector<ushort>, but CreateWhileLessThanMaskInt16 now returns Vector<short>. This is intentional per the design to have signed variants with signed return types.
Instruction Selection The JIT correctly selects WHILELT/WHILELE for signed inputs and WHILELO/WHILELS for unsigned inputs, as shown in the XML documentation

🔍 Questions for Author

  1. API Deprecation: Were the old APIs (CreateWhileLessThanMask8Bit, etc.) intentionally removed rather than deprecated? If existing code uses these APIs, it will break on upgrade. Consider whether [Obsolete] wrapper methods should be provided for a transition period.

  2. Signed Mask Semantics: For CreateWhileLessThanMaskInt16 returning Vector<short> vs the previous Vector<ushort> - does the bit pattern in the mask differ, or is this purely a type system change for better ergonomics when using with signed vector operations?


✅ Summary

Verdict: Looks good!

The PR is well-structured, consistent across all layers (JIT, managed code, tests, API compat), and the naming convention change improves API discoverability. The addition of signed and floating-point variants completes the API surface for these mask creation operations. The only consideration is whether a deprecation path for the old API names would be beneficial for users upgrading existing code.

@a74nh
Copy link
Contributor Author

a74nh commented Feb 10, 2026

  • API Deprecation: Were the old APIs (CreateWhileLessThanMask8Bit, etc.) intentionally removed rather than deprecated? If existing code uses these APIs, it will break on upgrade. Consider whether [Obsolete] wrapper methods should be provided for a transition period.

Intentionally removed - we're replacing it with the Byte version.
SVE is marked experimental, so it's fine to rename. I'll make sure to raise an API break issue.

  • Signed Mask Semantics: For CreateWhileLessThanMaskInt16 returning Vector<short> vs the previous Vector<ushort> - does the bit pattern in the mask differ, or is this purely a type system change for better ergonomics when using with signed vector operations?

Int16 has been replaced with both Int16 and UInt16 versions. In all three versions, the underlying bit pattern is identical.
We added this to avoid having to do casting in the C# code when you are using signed values.

@stephentoub
Copy link
Member

🤖 Copilot Code Review — PR #124081

Holistic Assessment

Motivation: This PR addresses issue #108233 by renaming SVE CreateWhileLessThan* and CreateWhileLessThanOrEqual* APIs from bit-width naming (e.g., CreateWhileLessThanMask8Bit) to type-specific naming (e.g., CreateWhileLessThanMaskByte, CreateWhileLessThanMaskSByte). This eliminates the need for awkward casts when using masks with signed or floating-point vector operations. The change is well-justified by the linked issue.

Approach: The implementation is thorough and consistent:

  • JIT intrinsic definitions and codegen updated
  • Managed API surface (Sve.cs and Sve.PlatformNotSupported.cs) aligned
  • Reference assembly updated
  • API compat baseline suppressions added for removed APIs
  • Test infrastructure and regression tests updated

Summary: ✅ LGTM. The PR is well-structured and the naming convention change improves API discoverability by aligning with existing SVE APIs like CreateTrueMaskByte, CreateTrueMaskDouble, etc. The breaking change is acceptable since SVE is marked experimental.


Detailed Findings

✅ Consistency — Naming aligns with existing SVE APIs

The new naming pattern (CreateWhileLessThanMaskByte, CreateWhileLessThanMaskInt16, CreateWhileLessThanMaskSingle, etc.) matches the established pattern used by CreateTrueMask* APIs:

  • CreateTrueMaskByte, CreateTrueMaskSByte
  • CreateTrueMaskInt16, CreateTrueMaskUInt16
  • CreateTrueMaskSingle, CreateTrueMaskDouble

✅ API Design — Complete type coverage

All 10 types are covered for both CreateWhileLessThan* and CreateWhileLessThanOrEqual*:

  • Signed integers: SByte, Int16, Int32, Int64
  • Unsigned integers: Byte, UInt16, UInt32, UInt64
  • Floating-point: Single, Double

Each variant has 4 overloads for the scalar operand types (int/long/uint/ulong).

✅ JIT Correctness — Instruction selection is correct

The codegen correctly selects:

  • WHILELT for signed operands
  • WHILELO for unsigned operands
  • WHILELE for signed operands (OrEqual variant)
  • WHILELS for unsigned operands (OrEqual variant)

The HW_Flag_SpecialCodeGen flag and SetAuxiliaryJitType pattern ensures the instruction is selected based on the actual operand type, not the return type.

✅ Breaking Change Handling — Properly documented

The old APIs are removed (not deprecated), which is appropriate for experimental APIs. The ApiCompatBaseline.NetCoreAppLatestStable.xml includes CP0002 suppressions for all 32 removed methods.

✅ Test Coverage — Comprehensive updates

  • Test generator (SveTests.cs) updated with all new variants including signed and floating-point types
  • Regression tests (Runtime_118377.cs, ChangeMaskUse.cs, MaskUse.cs) updated to use new API names

Summary

Verdict: ✅ LGTM

The PR improves API ergonomics by eliminating the need for explicit casts when working with signed or floating-point vectors. The implementation is consistent across all layers (JIT, managed code, ref assembly, tests). Since SVE is experimental, the breaking change to rename APIs is acceptable. No issues found.

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.

[API Proposal]: ARM64 SVE: Add additional types for the mask APIs

2 participants