Skip to content

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jan 2, 2025

Contributes to #94941.

Follow-up to #110953.

cc: @EgorBo

@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 2, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 2, 2025
@MihaZupan
Copy link
Member

I'm not a fan of this change, these types are very unsafe-adjacent so I don't see why repeating the keyword 7 times is beneficial.
If this were e.g. class HttpClient that happened to have one unsafe helper method in it, I'd feel differently.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 2, 2025

This change brings these files into alignment with Vector128, where an unsafe context is not used on the class.

@EgorBo
Copy link
Member

EgorBo commented Jan 2, 2025

I agree with @MihaZupan, but since there is already an inconsistency with V128 I guess it's fine to clean up?

PS: it is really unfortunate that sizeof requires unsafe context in C#

@MihaZupan
Copy link
Member

MihaZupan commented Jan 2, 2025

Looking at Vector128, a quick search shows 106 "static unsafe", but almost all of them look like they're unnecessary. Consistency is good, but maybe we should do the opposite and mark that type as unsafe too instead? Or perhaps remove the unneeded uses there.

@EgorBo
Copy link
Member

EgorBo commented Jan 2, 2025

Looking at Vector128, a quick search shows 106 "static unsafe", but almost all of them look like they're unnecessary. Consistency is good, but maybe we should do the opposite and mark that type as unsafe too instead? Or perhaps remove the unneeded uses there.

when I was removing redundant unsafe I didn't scan SIMD APIs so I can imagine there are some redundancies there

@stephentoub
Copy link
Member

stephentoub commented Jan 2, 2025

PS: it is really unfortunate that sizeof requires unsafe context in C#

I agree with this. As we move forward with refining exactly what "unsafe" means and adding to what triggers warnings and viral requirements, we should also look at removing things that are no longer appropriate.

@xtqqczze xtqqczze marked this pull request as ready for review January 2, 2025 14:56
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 2, 2025

PS: it is really unfortunate that sizeof requires unsafe context in C#

It also produces a compiler warning, which we suppress for the repository:

     Warning CS8500: This takes the address of, gets the size of, or declares a pointer to a managed type

static unsafe Vector128<T> ISimdVector<Vector128<T>, T>.LoadAlignedNonTemporal(T* source) => Vector128.LoadAlignedNonTemporal(source);

/// <inheritdoc cref="ISimdVector{TSelf, T}.LoadUnsafe(ref readonly T)" />
[Intrinsic]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overloads that take ref T should be really marked as unsafe too, even though C# does not require it today.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding it there is a bit unnecessary since it doesn't actually do anything. I think what we really want is [UnsafeCallersOnly] instead, so that callers must use unsafe { } to invoke the method.

@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
Copy link
Member

Looking at Vector128, a quick search shows 106 "static unsafe", but almost all of them look like they're unnecessary. Consistency is good, but maybe we should do the opposite and mark that type as unsafe too instead? Or perhaps remove the unneeded uses there.

I think we should likely close this PR until a desired approach is determined. The inconsistency isn't hurting anything and we don't have a definitive plan, so its effectively unnecessary churn.

I'd like to determine if we have concrete plans on pushing something like UnsafeCallersOnly through. In such a scenario, I imagine we would use that as the primary source of truth for whether something was considered "unsafe".

We then should make a separate determination on the coding style usage of unsafe on types/methods and whether we want to have it only on methods that actually use some kind of unsafe code or if having it on the type for unsafe adjacent concepts is acceptable.

After we decide the outcome of these points, then we can start submitting PRs that do such cleanup/normalization.

CC. @stephentoub, @jkotas

@xtqqczze
Copy link
Contributor Author

I'd like to determine if we have concrete plans on pushing something like UnsafeCallersOnly through. In such a scenario, I imagine we would use that as the primary source of truth for whether something was considered "unsafe".

Closing, blocked by:

@xtqqczze xtqqczze closed this Jan 29, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2025
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-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants