Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Obsolete invalid overloads of AdvSimd.ShiftRightLogicalRoundedNarrowingSaturate family #95525

Closed
saucecontrol opened this issue Dec 1, 2023 · 6 comments · Fixed by #99985
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.Intrinsics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Milestone

Comments

@saucecontrol
Copy link
Member

Background and motivation

The Arm Advanced SIMD UQRSHRN instruction performs an unsigned saturated narrow operation. As such, its result is always unsigned. However, the .NET intrinsics API for this instruction includes overloads that accept and return signed types.

These overloads will not work as expected if following the API description rather than the instruction description. It would likely be best to obsolete and hide them to avoid potential confusion.

API Proposal

 namespace System.Runtime.Intrinsics.Arm;

 public abstract class AdvSimd : ArmBase
 {
     public new abstract class Arm64 : ArmBase.Arm64
     {
         /// <summary>
         /// uint16_t vqrshrns_n_u32 (uint32_t a, const int n)
         ///   A64: UQRSHRN Hd, Sn, #n
         /// </summary>
+        [Obsolete("Use unsigned overload or signed instruction.")]
         public static Vector64<short> ShiftRightLogicalRoundedNarrowingSaturateScalar(Vector64<int> value, [ConstantExpected(Min = 1, Max = (byte)(16))] byte count) { throw new PlatformNotSupportedException(); }
  
         /// <summary>
         /// uint32_t vqrshrnd_n_u64 (uint64_t a, const int n)
         ///   A64: UQRSHRN Sd, Dn, #n
         /// </summary>
+        [Obsolete("Use unsigned overload or signed instruction.")]
         public static Vector64<int> ShiftRightLogicalRoundedNarrowingSaturateScalar(Vector64<long> value, [ConstantExpected(Min = 1, Max = (byte)(8))] byte count) { throw new PlatformNotSupportedException(); }
  
         /// <summary>
         /// uint8_t vqrshrnh_n_u16 (uint16_t a, const int n)
         ///   A64: UQRSHRN Bd, Hn, #n
         /// </summary>
+        [Obsolete("Use unsigned overload or signed instruction.")]
         public static Vector64<sbyte> ShiftRightLogicalRoundedNarrowingSaturateScalar(Vector64<short> value, [ConstantExpected(Min = 1, Max = (byte)(32))] byte count) { throw new PlatformNotSupportedException(); }
     }
 
     /// <summary>
     /// uint16x4_t vqrshrn_n_u32 (uint32x4_t a, const int n)
     ///   A32: VQRSHRN.U32 Dd, Qm, #n
     ///   A64: UQRSHRN Vd.4H, Vn.4S, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector64<short> ShiftRightLogicalRoundedNarrowingSaturateLower(Vector128<int> value, [ConstantExpected(Min = 1, Max = (byte)(32))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint32x2_t vqrshrn_n_u64 (uint64x2_t a, const int n)
     ///   A32: VQRSHRN.U64 Dd, Qm, #n
     ///   A64: UQRSHRN Vd.2S, Vn.2D, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector64<int> ShiftRightLogicalRoundedNarrowingSaturateLower(Vector128<long> value, [ConstantExpected(Min = 1, Max = (byte)(16))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint8x8_t vqrshrn_n_u16 (uint16x8_t a, const int n)
     ///   A32: VQRSHRN.U16 Dd, Qm, #n
     ///   A64: UQRSHRN Vd.8B, Vn.8H, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector64<sbyte> ShiftRightLogicalRoundedNarrowingSaturateLower(Vector128<short> value, [ConstantExpected(Min = 1, Max = (byte)(64))] byte count) { throw new PlatformNotSupportedException(); }
 
     /// <summary>
     /// uint16x8_t vqrshrn_high_n_u32 (uint16x4_t r, uint32x4_t a, const int n)
     ///   A32: VQRSHRN.U32 Dd+1, Dn, #n
     ///   A64: UQRSHRN2 Vd.8H, Vn.4S, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector128<short> ShiftRightLogicalRoundedNarrowingSaturateUpper(Vector64<short> lower, Vector128<int> value, [ConstantExpected(Min = 1, Max = (byte)(32))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint32x4_t vqrshrn_high_n_u64 (uint32x2_t r, uint64x2_t a, const int n)
     ///   A32: VQRSHRN.U64 Dd+1, Dn, #n
     ///   A64: UQRSHRN2 Vd.4S, Vn.2D, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector128<int> ShiftRightLogicalRoundedNarrowingSaturateUpper(Vector64<int> lower, Vector128<long> value, [ConstantExpected(Min = 1, Max = (byte)(16))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint8x16_t vqrshrn_high_n_u16 (uint8x8_t r, uint16x8_t a, const int n)
     ///   A32: VQRSHRN.U16 Dd+1, Dn, #n
     ///   A64: UQRSHRN2 Vd.16B, Vn.8H, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector128<sbyte> ShiftRightLogicalRoundedNarrowingSaturateUpper(Vector64<sbyte> lower, Vector128<short> value, [ConstantExpected(Min = 1, Max = (byte)(64))] byte count) { throw new PlatformNotSupportedException(); }
 }

API Usage

N/A

Alternative Designs

Could just leave them there and let people trip on them.

Risks

Source-breaking change.

@saucecontrol saucecontrol added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 1, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 1, 2023
@ghost
Copy link

ghost commented Dec 1, 2023

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

Issue Details

Background and motivation

The Arm Advanced SIMD UQRSHRN instruction performs an unsigned saturated narrow operation. As such, its result is always unsigned. However, the .NET intrinsics API for this instruction includes overloads that accept and return signed types.

These overloads will not work as expected if following the API description rather than the instruction description. It would likely be best to obsolete and hide them to avoid potential confusion.

API Proposal

 namespace System.Runtime.Intrinsics.Arm;

 public abstract class AdvSimd : ArmBase
 {
     public new abstract class Arm64 : ArmBase.Arm64
     {
         /// <summary>
         /// uint16_t vqrshrns_n_u32 (uint32_t a, const int n)
         ///   A64: UQRSHRN Hd, Sn, #n
         /// </summary>
+        [Obsolete("Use unsigned overload or signed instruction.")]
         public static Vector64<short> ShiftRightLogicalRoundedNarrowingSaturateScalar(Vector64<int> value, [ConstantExpected(Min = 1, Max = (byte)(16))] byte count) { throw new PlatformNotSupportedException(); }
  
         /// <summary>
         /// uint32_t vqrshrnd_n_u64 (uint64_t a, const int n)
         ///   A64: UQRSHRN Sd, Dn, #n
         /// </summary>
+        [Obsolete("Use unsigned overload or signed instruction.")]
         public static Vector64<int> ShiftRightLogicalRoundedNarrowingSaturateScalar(Vector64<long> value, [ConstantExpected(Min = 1, Max = (byte)(8))] byte count) { throw new PlatformNotSupportedException(); }
  
         /// <summary>
         /// uint8_t vqrshrnh_n_u16 (uint16_t a, const int n)
         ///   A64: UQRSHRN Bd, Hn, #n
         /// </summary>
+        [Obsolete("Use unsigned overload or signed instruction.")]
         public static Vector64<sbyte> ShiftRightLogicalRoundedNarrowingSaturateScalar(Vector64<short> value, [ConstantExpected(Min = 1, Max = (byte)(32))] byte count) { throw new PlatformNotSupportedException(); }
     }
 
     /// <summary>
     /// uint16x4_t vqrshrn_n_u32 (uint32x4_t a, const int n)
     ///   A32: VQRSHRN.U32 Dd, Qm, #n
     ///   A64: UQRSHRN Vd.4H, Vn.4S, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector64<short> ShiftRightLogicalRoundedNarrowingSaturateLower(Vector128<int> value, [ConstantExpected(Min = 1, Max = (byte)(32))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint32x2_t vqrshrn_n_u64 (uint64x2_t a, const int n)
     ///   A32: VQRSHRN.U64 Dd, Qm, #n
     ///   A64: UQRSHRN Vd.2S, Vn.2D, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector64<int> ShiftRightLogicalRoundedNarrowingSaturateLower(Vector128<long> value, [ConstantExpected(Min = 1, Max = (byte)(16))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint8x8_t vqrshrn_n_u16 (uint16x8_t a, const int n)
     ///   A32: VQRSHRN.U16 Dd, Qm, #n
     ///   A64: UQRSHRN Vd.8B, Vn.8H, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector64<sbyte> ShiftRightLogicalRoundedNarrowingSaturateLower(Vector128<short> value, [ConstantExpected(Min = 1, Max = (byte)(64))] byte count) { throw new PlatformNotSupportedException(); }
 
     /// <summary>
     /// uint16x8_t vqrshrn_high_n_u32 (uint16x4_t r, uint32x4_t a, const int n)
     ///   A32: VQRSHRN.U32 Dd+1, Dn, #n
     ///   A64: UQRSHRN2 Vd.8H, Vn.4S, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector128<short> ShiftRightLogicalRoundedNarrowingSaturateUpper(Vector64<short> lower, Vector128<int> value, [ConstantExpected(Min = 1, Max = (byte)(32))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint32x4_t vqrshrn_high_n_u64 (uint32x2_t r, uint64x2_t a, const int n)
     ///   A32: VQRSHRN.U64 Dd+1, Dn, #n
     ///   A64: UQRSHRN2 Vd.4S, Vn.2D, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector128<int> ShiftRightLogicalRoundedNarrowingSaturateUpper(Vector64<int> lower, Vector128<long> value, [ConstantExpected(Min = 1, Max = (byte)(16))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint8x16_t vqrshrn_high_n_u16 (uint8x8_t r, uint16x8_t a, const int n)
     ///   A32: VQRSHRN.U16 Dd+1, Dn, #n
     ///   A64: UQRSHRN2 Vd.16B, Vn.8H, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector128<sbyte> ShiftRightLogicalRoundedNarrowingSaturateUpper(Vector64<sbyte> lower, Vector128<short> value, [ConstantExpected(Min = 1, Max = (byte)(64))] byte count) { throw new PlatformNotSupportedException(); }
 }

API Usage

N/A

Alternative Designs

Could just leave them there and let people trip on them.

Risks

Source-breaking change.

Author: saucecontrol
Assignees: -
Labels:

api-suggestion, area-System.Runtime.Intrinsics

Milestone: -

@huoyaoyuan
Copy link
Member

Vector256.ShiftRightArithmetic is only defined for signed types. However ShiftRightLogical is defined for both signed and unsigned types. So SRL is considered a valid operation on signed type. The new C# operator >>> (op_UnsignedRightShift) is created specifically for that.

@saucecontrol
Copy link
Member Author

It's not the shift that's the problem, it's the unsigned saturated narrow. The result literally cannot be a signed value -- it will be saturated to a min of zero.

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Dec 4, 2023
@tannergooding tannergooding added this to the 9.0.0 milestone Dec 4, 2023
@bartonjs
Copy link
Member

bartonjs commented Jan 9, 2024

Video

  • The error message should be improved, but the obsoletions themselves are approved.
  • All of them should use the same new diagnostic code.
 namespace System.Runtime.Intrinsics.Arm;

 public abstract class AdvSimd : ArmBase
 {
     public new abstract class Arm64 : ArmBase.Arm64
     {
         /// <summary>
         /// uint16_t vqrshrns_n_u32 (uint32_t a, const int n)
         ///   A64: UQRSHRN Hd, Sn, #n
         /// </summary>
+        [Obsolete("Use unsigned overload or signed instruction.")]
         public static Vector64<short> ShiftRightLogicalRoundedNarrowingSaturateScalar(Vector64<int> value, [ConstantExpected(Min = 1, Max = (byte)(16))] byte count) { throw new PlatformNotSupportedException(); }
  
         /// <summary>
         /// uint32_t vqrshrnd_n_u64 (uint64_t a, const int n)
         ///   A64: UQRSHRN Sd, Dn, #n
         /// </summary>
+        [Obsolete("Use unsigned overload or signed instruction.")]
         public static Vector64<int> ShiftRightLogicalRoundedNarrowingSaturateScalar(Vector64<long> value, [ConstantExpected(Min = 1, Max = (byte)(8))] byte count) { throw new PlatformNotSupportedException(); }
  
         /// <summary>
         /// uint8_t vqrshrnh_n_u16 (uint16_t a, const int n)
         ///   A64: UQRSHRN Bd, Hn, #n
         /// </summary>
+        [Obsolete("Use unsigned overload or signed instruction.")]
         public static Vector64<sbyte> ShiftRightLogicalRoundedNarrowingSaturateScalar(Vector64<short> value, [ConstantExpected(Min = 1, Max = (byte)(32))] byte count) { throw new PlatformNotSupportedException(); }
     }
 
     /// <summary>
     /// uint16x4_t vqrshrn_n_u32 (uint32x4_t a, const int n)
     ///   A32: VQRSHRN.U32 Dd, Qm, #n
     ///   A64: UQRSHRN Vd.4H, Vn.4S, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector64<short> ShiftRightLogicalRoundedNarrowingSaturateLower(Vector128<int> value, [ConstantExpected(Min = 1, Max = (byte)(32))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint32x2_t vqrshrn_n_u64 (uint64x2_t a, const int n)
     ///   A32: VQRSHRN.U64 Dd, Qm, #n
     ///   A64: UQRSHRN Vd.2S, Vn.2D, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector64<int> ShiftRightLogicalRoundedNarrowingSaturateLower(Vector128<long> value, [ConstantExpected(Min = 1, Max = (byte)(16))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint8x8_t vqrshrn_n_u16 (uint16x8_t a, const int n)
     ///   A32: VQRSHRN.U16 Dd, Qm, #n
     ///   A64: UQRSHRN Vd.8B, Vn.8H, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector64<sbyte> ShiftRightLogicalRoundedNarrowingSaturateLower(Vector128<short> value, [ConstantExpected(Min = 1, Max = (byte)(64))] byte count) { throw new PlatformNotSupportedException(); }
 
     /// <summary>
     /// uint16x8_t vqrshrn_high_n_u32 (uint16x4_t r, uint32x4_t a, const int n)
     ///   A32: VQRSHRN.U32 Dd+1, Dn, #n
     ///   A64: UQRSHRN2 Vd.8H, Vn.4S, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector128<short> ShiftRightLogicalRoundedNarrowingSaturateUpper(Vector64<short> lower, Vector128<int> value, [ConstantExpected(Min = 1, Max = (byte)(32))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint32x4_t vqrshrn_high_n_u64 (uint32x2_t r, uint64x2_t a, const int n)
     ///   A32: VQRSHRN.U64 Dd+1, Dn, #n
     ///   A64: UQRSHRN2 Vd.4S, Vn.2D, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector128<int> ShiftRightLogicalRoundedNarrowingSaturateUpper(Vector64<int> lower, Vector128<long> value, [ConstantExpected(Min = 1, Max = (byte)(16))] byte count) { throw new PlatformNotSupportedException(); }
  
     /// <summary>
     /// uint8x16_t vqrshrn_high_n_u16 (uint8x8_t r, uint16x8_t a, const int n)
     ///   A32: VQRSHRN.U16 Dd+1, Dn, #n
     ///   A64: UQRSHRN2 Vd.16B, Vn.8H, #n
     /// </summary>
+    [Obsolete("Use unsigned overload or signed instruction.")]
     public static Vector128<sbyte> ShiftRightLogicalRoundedNarrowingSaturateUpper(Vector64<sbyte> lower, Vector128<short> value, [ConstantExpected(Min = 1, Max = (byte)(64))] byte count) { throw new PlatformNotSupportedException(); }
 }

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 9, 2024
@tannergooding tannergooding added the help wanted [up-for-grabs] Good issue for external contributors label Feb 8, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Mar 19, 2024
@jeffhandley jeffhandley added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Mar 19, 2024
@dotnet-policy-service dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Mar 19, 2024

This comment was marked as resolved.

@github-actions github-actions bot locked and limited conversation to collaborators May 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.Intrinsics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants