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

JIT: Added Sve.LoadVectorNonTemporal/NonFaulting/128AndReplicateToVector APIs #103392

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Jun 13, 2024

Contributes to #99957

Adds:

  • LoadVectorNonTemporal
  • LoadVectorNonFaulting
  • LoadVector128AndReplicateToVector

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.

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.

@TIHan
Copy link
Contributor Author

TIHan commented Jun 13, 2024

sve_load_vector_results.txt

@dotnet/arm64-contrib @kunalspathak this is ready. The test results do have failures for NonTemporal and 128AndReplicateToVector because of predicate register support.

@@ -0,0 +1,270 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file is doing the same job as SveLoadMaskedUnOpTest.template, which is being used for
Vector<byte> LoadVector(Vector<byte> mask, byte* address) etc.

Your new file is better. Can you combine them both into a single template? (either naming is fine)

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I wanted this, but it's a bit more difficult because the other Load APIs take different operand types. Can we do that one in another PR?

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.

Overall looks great. Added few questions and comments about test.

src/coreclr/jit/hwintrinsiclistarm64sve.h Outdated Show resolved Hide resolved
src/coreclr/jit/hwintrinsiclistarm64sve.h Outdated Show resolved Hide resolved
@@ -0,0 +1,270 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

+1

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. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants