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.

Sign up for GitHub

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

Jump to bottom

JIT: SVE Cleanup - Remove unsafe from APIs that do not use pointers. #104845

Closed
TIHan opened this issue Jul 13, 2024 · 5 comments · Fixed by #107854
Closed

JIT: SVE Cleanup - Remove unsafe from APIs that do not use pointers. #104845

TIHan opened this issue Jul 13, 2024 · 5 comments · Fixed by #107854
Assignees
@SwapnilGaikwad
Labels
arch-arm64 area-System.Runtime.Intrinsics arm-sve Work related to arm64 SVE/SVE2 support in-pr There is an active PR which will close this issue when it is merged Priority:2 Work that is important, but not critical for the release
Milestone
10.0.0

Comments

@TIHan
Copy link
Contributor

TIHan commented Jul 13, 2024

Because we have been copying and pasting from the generated code for SVE, the generation always marked the APIs as unsafe. We need to remove unsafe from them if the API does not use any pointers.

Relevant files:

  • src\libraries\System.Runtime.Intrinsics\ref\System.Runtime.Intrinsics.cs
  • src\libraries\System.Private.CoreLib\src\System\Runtime\Intrinsics\Arm\Sve.PlatformNotSupported.cs
  • src\System\Runtime\Intrinsics\Arm\Sve.cs
@TIHan TIHan added this to the 9.0.0 milestone Jul 13, 2024
@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 Jul 13, 2024
@vcsjones vcsjones added area-System.Runtime.Intrinsics and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 13, 2024
@dotnet-policy-service 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.

@jeffhandley jeffhandley added the arm-sve Work related to arm64 SVE/SVE2 support label Jul 28, 2024
@a74nh
Copy link
Contributor

a74nh commented Jul 29, 2024

I recommend doing this after making the files alphabetical in #104834

@a74nh a74nh added the Priority:2 Work that is important, but not critical for the release label Jul 30, 2024
@a74nh
Copy link
Contributor

a74nh commented Jul 30, 2024

priority:2 for RC1 snap : User visible API

@tannergooding
Copy link
Member

tannergooding commented Jul 30, 2024 •
edited
Loading

This is more rather priority 3, or could even be pushed out to .NET 10.

Marking an API as unsafe has no impact to the public signature, it simply allows the internal implementation to use pointers and the public signature to contain pointers. It's then only if the public signature contains pointers that the downstream consumers must also themselves use unsafe.

In many cases we've just marked the entire class as unsafe to simplify long term revisions/changes to known lowlevel code.

@JulieLeeMSFT
Copy link
Member

Pushing out to .NET 10 because we have many issues for RC1 snap.

@JulieLeeMSFT JulieLeeMSFT modified the milestones: 9.0.0, 10.0.0 Jul 30, 2024
SwapnilGaikwad added a commit to SwapnilGaikwad/runtime that referenced this issue Sep 16, 2024
@SwapnilGaikwad
Remove redundant unsafe from APIs
225f82c 
Fixes: dotnet#104845
@SwapnilGaikwad SwapnilGaikwad mentioned this issue Sep 16, 2024
Merged
@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 Sep 16, 2024
kunalspathak pushed a commit that referenced this issue Sep 16, 2024
@SwapnilGaikwad
Remove redundant unsafe from SVE APIs (#107854)
1bde882 
* Remove redundant unsafe from APIs

Fixes: #104845

* Restore incorrectly commented API
jtschuster pushed a commit to jtschuster/runtime that referenced this issue Sep 17, 2024
@SwapnilGaikwad @jtschuster
Remove redundant unsafe from SVE APIs (dotnet#107854)
05327bf 
* Remove redundant unsafe from APIs

Fixes: dotnet#104845

* Restore incorrectly commented API
sirntar pushed a commit to sirntar/runtime that referenced this issue Sep 30, 2024
@SwapnilGaikwad @sirntar
Remove redundant unsafe from SVE APIs (dotnet#107854)
e29a4b7 
* Remove redundant unsafe from APIs

Fixes: dotnet#104845

* Restore incorrectly commented API
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2024
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this issue Dec 10, 2024
@SwapnilGaikwad @mikelle-rogers
Remove redundant unsafe from SVE APIs (dotnet#107854)
3d7da5b 
* Remove redundant unsafe from APIs

Fixes: dotnet#104845

* Restore incorrectly commented API
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@SwapnilGaikwad SwapnilGaikwad

Labels
arch-arm64 area-System.Runtime.Intrinsics arm-sve Work related to arm64 SVE/SVE2 support in-pr There is an active PR which will close this issue when it is merged Priority:2 Work that is important, but not critical for the release
Projects
None yet
Milestone
10.0.0
Development

Successfully merging a pull request may close this issue.

Remove redundant unsafe from SVE APIs SwapnilGaikwad/runtime
7 participants
@vcsjones @jeffhandley @TIHan @a74nh @SwapnilGaikwad @tannergooding @JulieLeeMSFT