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

Minimize use of unsafe context #111025

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jan 2, 2025

Contributes to #94941.

Follow-up to #110953.

cc: @EgorBo

@dotnet-issue-labeler dotnet-issue-labeler bot 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


/// <inheritdoc cref="ISimdVector{TSelf, T}.LoadAlignedNonTemporal(T*)" />
[Intrinsic]
static Vector128<T> ISimdVector<Vector128<T>, T>.LoadAlignedNonTemporal(T* source) => Vector128.LoadAlignedNonTemporal(source);
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants