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

ARM64-SVE: Add GatherVectorWithByteOffsetFirstFaulting #106199

Merged
merged 2 commits into from
Aug 12, 2024
Merged

ARM64-SVE: Add GatherVectorWithByteOffsetFirstFaulting #106199

merged 2 commits into from
Aug 12, 2024

Conversation

amanasifkhalid
Copy link
Member

Part of #99957. Adds a new test template for the ByteOffset first-faulting APIs. All stress tests are passing.

@dotnet/arm64-contrib PTAL, thanks!

@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Aug 9, 2024
@dotnet-issue-labeler dotnet-issue-labeler
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
@dotnet-issue-labeler dotnet-issue-labeler
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.

This was referenced Aug 9, 2024
Closed
Closed
@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Aug 9, 2024
@amanasifkhalid @jakobbotsch
Use HW_Flag_SpecialSideEffect_Other
86467ec 
Co-authored-by: Jakob Botsch Nielsen <[email protected]>
@amanasifkhalid amanasifkhalid added the arm-sve Work related to arm64 SVE/SVE2 support label Aug 9, 2024
This was referenced Aug 9, 2024
Open
Closed
@amanasifkhalid amanasifkhalid mentioned this pull request Aug 9, 2024
Merged
a74nh
a74nh approved these changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@a74nh a74nh left a comment

Choose a reason for hiding this comment

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

This LGTM

@amanasifkhalid
Copy link
Member Author

Thanks for the review! @TIHan could you PTAL? Thanks

AndyAyersMS
AndyAyersMS approved these changes Aug 12, 2024
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Changes look good to me, though I am not familiar with much of this.

kunalspathak
kunalspathak reviewed Aug 12, 2024
View reviewed changes
src/tests/JIT/HardwareIntrinsics/Arm/Shared/SveGatherVectorByteOffsetFirstFaulting.template
// sel mask, target, target, falseReg
//
// This needs more careful thinking, so disabling it for now.
// test.ConditionalSelect_FalseOp();
Copy link
Member

Choose a reason for hiding this comment

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

Will you send a follow-up PR to uncomment this once #105737 is merged or can you merge #105737 first and uncomment this one?

Copy link
Member Author

@amanasifkhalid amanasifkhalid Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This scenario is also disabled in SveGatherVectorFirstFaultingIndices.template and SveGatherVectorFirstFaultingVectorBases.template, so maybe we can re-enable it across all templates as a follow-up to #105737? I can create an issue to track it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created an issue for this: #106279

kunalspathak
kunalspathak approved these changes Aug 12, 2024
View reviewed changes
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 modulo the comment about one of the test case.

@amanasifkhalid amanasifkhalid merged commit 6169e41 into dotnet:main Aug 12, 2024
168 of 171 checks passed
@amanasifkhalid amanasifkhalid deleted the sve-gather-ff branch August 12, 2024 17:08
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jakobbotsch jakobbotsch jakobbotsch left review comments

@a74nh a74nh a74nh approved these changes

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

@kunalspathak kunalspathak kunalspathak approved these changes

Assignees

@amanasifkhalid amanasifkhalid

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI arm-sve Work related to arm64 SVE/SVE2 support new-api-needs-documentation
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@amanasifkhalid @a74nh @jakobbotsch @AndyAyersMS @kunalspathak @JulieLeeMSFT