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

Ensure ISimdVector default implementations throw documented exceptions #104831

Closed
wants to merge 3 commits into from

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jul 12, 2024

No description provided.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 12, 2024
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.

@xtqqczze
Copy link
Contributor Author

@MihuBot

@xtqqczze
Copy link
Contributor Author

sharplab

@EgorBo
Copy link
Member

EgorBo commented Jul 17, 2024

sharplab

it's generally not the right strategy to massage C# code and duplicate things just to achieve a slightly better codegen, we should go with the most clean version and file an issue against JIT if something is suboptimal IMO

@tannergooding
Copy link
Member

it's generally not the right strategy to massage C# code and duplicate things just to achieve a slightly better codegen, we should go with the most clean version and file an issue against JIT if something is suboptimal IMO

👍

This should really just be TSelf.CopyTo(vector, destination.AsSpan(0, destination.Length)) with a small comment that we do AsSpan this way so we get the nullcheck

@xtqqczze xtqqczze changed the title Ensure ISimdVector default implementations perform nullcheck Ensure ISimdVector default implementations throw documented exceptions Jul 17, 2024
@xtqqczze
Copy link
Contributor Author

This should really just be TSelf.CopyTo(vector, destination.AsSpan(0, destination.Length)) with a small comment that we do AsSpan this way so we get the nullcheck

Unfortunately AsSpan(0, destination.Length) would not throw the specifically documented exceptions which would result in behaviour inconsistent with existing explicit interface implementations. I will update the PR description.

@tannergooding
Copy link
Member

Unfortunately AsSpan(0, destination.Length) would not throw the specifically documented exceptions which would result in behaviour inconsistent with existing explicit interface implementations.

What do you mean? This would specifically fault on destination.Length and cause the NullReferenceException to be thrown

@xtqqczze
Copy link
Contributor Author

What do you mean? This would specifically fault on destination.Length and cause the NullReferenceException to be thrown

I mean the implementations make a distinction between ArgumentOutOfRangeException and ArgumentException, this is documented in ISimdVector.

@tannergooding
Copy link
Member

Still not seeing the issue here.

static void CopyTo<T>(this Vector128<T> vector, Span<T> destination) will:

  • throw NotSupportedException with a message similar to Specified type is not supported if T is not supported
  • throw ArgumentException with a message similar to Destination is too short. if destination.Length < Vector128<T>.Count

static void CopyTo<T>(this Vector128<T> vector, T[] destination) will:

  • throw the exact same exceptions as the span overload
  • additionally will throw NullReferenceException if destination is null

static void CopyTo<T>(this Vector128<T> vector, T[] destination, int startIndex) will:

  • throw the exact same exceptions as the array overload
  • additionally will throw ArgumentOutOfRangeException with a message similar to Index was out of range. Must be non-negative and less than the size of the collection. if startIndex is negative or greater than destination.Length

static Span<T> AsSpan<T>(this T[]? array, int start, int length) will:

  • throw ArrayTypeMismatchException if the array is not actually a T[]
  • throw ArgumentOutOfRangeException with the default message if start is negative or greater than destination.Length and if length is negative or greater than array.Length - start

Thus, having the T[] destination version defer down to TSelf.CopyTo(vector, destination.AsSpan(0, destination.Length)) is exactly identical in behavior

Having the T[] destination, int startIndex version defer to to TSelf.CopyTo(vector, destination.AsSpan(startIndex, destination.Length - startIndex)) would have nearly identical behavior only differing in the message content of ArgumentOutOfRangeException which isn't itself guaranteed.

@stephentoub stephentoub added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 22, 2024
Copy link
Contributor

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

Copy link
Contributor

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.Intrinsics community-contribution Indicates that the PR has been added by a community member needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants