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

Add support for Sve.UnzipEven/Odd & Sve.ZipHighLow #101294

Merged
merged 5 commits into from
Apr 26, 2024

Conversation

SwapnilGaikwad
Copy link
Contributor

Add SVE API for unpredicated versions of UnzipEven, UnzipOdd, ZipHigh & ZipLow methods.

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

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

@SwapnilGaikwad
Copy link
Contributor Author

On a V1 system with SVE vector length 256 bits, tests for this patch would fail. We are currently limited to vector length of 128 causing incorrect results on systems with higher vector lengths. We need #101295 to limit vector length to 128 bits.

@SwapnilGaikwad SwapnilGaikwad marked this pull request as ready for review April 19, 2024 14:41
@SwapnilGaikwad
Copy link
Contributor Author

@a74nh @kunalspathak @dotnet/arm64-contrib @arch-arm64-sve

@JulieLeeMSFT
Copy link
Member

Contributes to #99957

@JulieLeeMSFT
Copy link
Member

Thanks @SwapnilGaikwad for the PR for bitmanipulate. When you merge the PR, could you check off the boxes from #99957?

@kunalspathak kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label Apr 21, 2024
@kunalspathak kunalspathak self-requested a review April 21, 2024 18:15
@kunalspathak
Copy link
Member

We need #101295 to limit vector length to 128 bits.

Is your plan to always limit the vector length to 128 bits? If yes, how can we test the functionality on 256 bits and higher?

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

the changes looks good otherwise, except the comment about testing.

@SwapnilGaikwad
Copy link
Contributor Author

Thanks @SwapnilGaikwad for the PR for bitmanipulate. When you merge the PR, could you check off the boxes from #99957?

I'll post a comment on the issue as I cannot edit the text of the issue 👍

@SwapnilGaikwad
Copy link
Contributor Author

We need #101295 to limit vector length to 128 bits.

Is your plan to always limit the vector length to 128 bits? If yes, how can we test the functionality on 256 bits and higher?

This just for temporary workaround until we support higher vector lengths using Vector. Currently it's limited to 128bits. These changes are tested for higher vector lengths locally. Once we start supporting higher vector lengths with Vector, we don't need to limit the vector lengths using the mentioned patch. There won't be any changes required to this patch then.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

@kunalspathak
Copy link
Member

The only failure that BuildAnalysis didn't tag as "known issue" is actually #91757 and not sure why it didn't get tagged. cc: @JulieLeeMSFT

@kunalspathak
Copy link
Member

/ba-g #91757

/// svbool_t svuzp1_b8(svbool_t op1, svbool_t op2)
/// UZP1 Presult.B, Pop1.B, Pop2.B
/// </summary>
public static unsafe Vector<byte> UnzipEven(Vector<byte> left, Vector<byte> right) => UnzipEven(left, right);
Copy link
Member

Choose a reason for hiding this comment

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

I just realized that zip and unzip instructions operate on both vector and predicate registers. How do we know which one to invoke? Currently, we will always invoke vector variants? @a74nh ?

If we always going to support just the vector version, then please remove the comment for predicate registers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now it'll just be the vector variant. If we're using masks then there will be a lot of conversions and the vector version will be used.

Raised a ticket: #101598

In the meantime, is it ok to leave the comment as is?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for opening an issue and yes, ok to keep it as of now.

@kunalspathak kunalspathak merged commit 9892d46 into dotnet:main Apr 26, 2024
160 of 168 checks passed
@SwapnilGaikwad SwapnilGaikwad deleted the github-sve-zip branch April 26, 2024 14:53
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* Add support for Sve.UnzipEven/Odd & Sve.ZipHighLow

* Rename the test template

---------

Co-authored-by: Kunal Pathak <[email protected]>
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* Add support for Sve.UnzipEven/Odd & Sve.ZipHighLow

* Rename the test template

---------

Co-authored-by: Kunal Pathak <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.Intrinsics arm-sve Work related to arm64 SVE/SVE2 support community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants