Skip to content

Simplify SVE unary test template #118129

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
Merged
amanasifkhalid merged 4 commits into from
Jul 29, 2025
Merged

Simplify SVE unary test template #118129

amanasifkhalid merged 4 commits into from
Jul 29, 2025

Conversation

@snickolls-arm
Copy link
Contributor

@snickolls-arm snickolls-arm commented Jul 28, 2025

  • Replace DataTable with PinnedVector class
  • Add functions for generating random Vectors
  • Add functions for converting between Vector and Array types
  • Generate an expected Vector for comparison and log to terminal
  • Simplify validation functions
  • Remove RetVectorType and Op1VectorType template variables

This patch is aiming to extract out some of the common code written in each of the templates to make them easier to debug and change in future, starting with the simpler Unary SVE template.

@snickolls-arm
Simplify SVE unary test template
d87cb01 
* Replace DataTable with PinnedVector class
* Add functions for generating random Vectors
* Add functions for converting between Vector and Array types
* Generate an expected Vector for comparison and log to terminal
* Simplify validation functions
* Remove RetVectorType and Op1VectorType template variables
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 28, 2025
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 28, 2025
@snickolls-arm
Copy link
Contributor Author

snickolls-arm commented Jul 28, 2025

@dotnet/arm64-contrib

amanasifkhalid
amanasifkhalid reviewed Jul 28, 2025
View reviewed changes
Copy link
Contributor

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

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

Thanks for taking on this cleanup! I have a few notes on reducing Unsafe.* usage.

amanasifkhalid
amanasifkhalid reviewed Jul 28, 2025
View reviewed changes
Copy link
Contributor

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

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

Thanks for taking on this cleanup! I have a few notes on reducing Unsafe.* usage.

@snickolls-arm
* Use Span type to copy data
0ff37d4 
* Remove unnecessary initializations
* Remove unused ArrayToVector function
* Fix exception thrown in SignExtend helper
@snickolls-arm
Fix ReciprocalExponent helper errors
e410c2f
amanasifkhalid
amanasifkhalid approved these changes Jul 29, 2025
View reviewed changes
Copy link
Contributor

@amanasifkhalid amanasifkhalid left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. I assume you're planning on removing all the now-unused templates -- Op1VectorType and TemplateValidationLogicForCndSel -- in a subsequent PR, right? never mind that, I see they're still used by all the other templates

@snickolls-arm
Copy link
Contributor Author

snickolls-arm commented Jul 29, 2025

LGTM. I assume you're planning on removing all the now-unused templates -- Op1VectorType and TemplateValidationLogicForCndSel -- in a subsequent PR, right? never mind that, I see they're still used by all the other templates

I'm primarily intending to deprecate these variables so the rest of the SVE2 tests we're going to add can be simpler. But if I have time to clean up the existing data after editing the templates then I would like to do so as well.

@snickolls-arm
Copy link
Contributor Author

snickolls-arm commented Jul 29, 2025

I also noticed a failure in the first build related to Sve.ReciprocalExponent. I've pushed what I think will fix this to this branch but it may not show again since the test data is random. It didn't seem to show the problem in this second build.

@amanasifkhalid amanasifkhalid enabled auto-merge (squash) July 29, 2025 18:36
@amanasifkhalid amanasifkhalid merged commit 97d9422 into dotnet:main Jul 29, 2025
73 of 75 checks passed
@build-analysis build-analysis bot mentioned this pull request Jul 29, 2025
Open
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@a74nh a74nh a74nh left review comments

@amanasifkhalid amanasifkhalid amanasifkhalid approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

arch-arm64 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@snickolls-arm @a74nh @amanasifkhalid