Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ARM64-SVE: GatherVectorWithByteOffsets #103564

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

a74nh
Copy link
Contributor

@a74nh a74nh commented Jun 17, 2024

This is a standard gatherload, but the offset is measured in bytes rather than the size of the vector. Therefore testing will have byte array, but needs to load a 32/64bit value.

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.

1 similar comment
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 Jun 17, 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.

@a74nh
Copy link
Contributor Author

a74nh commented Jun 17, 2024

Stress results - failures looks like they are the usual stress predicate issue.

@a74nh
Copy link
Contributor Author

a74nh commented Jun 17, 2024

Last of the gather loads. @dotnet/arm64-contrib @kunalspathak

@a74nh a74nh marked this pull request as ready for review June 17, 2024 10:14
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.

Added few comments around testing.

private void ValidateResult({Op1VectorType}<{Op1BaseType}> op1, {Op2BaseType}* op2, {Op3VectorType}<{Op3BaseType}> op3, void* result, [CallerMemberName] string method = "")
{
{Op1BaseType}[] inArray1 = new {Op1BaseType}[Op1ElementCount];
byte[] inArray2 = new byte[Unsafe.SizeOf<{Op2BaseType}>() * Op2ElementCount];
Copy link
Member

Choose a reason for hiding this comment

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

This template is identical to SveGatherVectorIndices.template except that byte[] vs. {Op2BaseType[]} and wondering if we add an argument like IndexType or something that is same as Op2BaseType for *Indices* and is byte for *Offset?

Copy link
Contributor Author

@a74nh a74nh Jun 18, 2024

Choose a reason for hiding this comment

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

Three differences between the two:

  • byte[] vs. {Op2BaseType[]}
  • In ValidateResult functions, the byte array needs to be of size Unsafe.SizeOf<{Op2BaseType}>() * Op2ElementCount instead of Op2ElementCount.
  • The result is validated by using a helper function Load{RetBaseType}FromByteArray instead of loading using array notation secondOp[thirdOp[i]]

Agreed, the first one could be switched easily, but I think the second two would be confusing if generic.

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

/ba-g timeout issues

@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
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.

2 participants