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

[release/9.0] JIT ARM64-SVE: Backport "Avoid combining conditional select for reduction instrinsics" (#107207) #107722

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

amanasifkhalid
Copy link
Member

Backport of #107207 to release/9.0

/cc @kunalspathak @SwapnilGaikwad

Customer Impact

  • Customer reported
  • Found internally

For ARM SVE reduction intrinsics, or intrinsics that produce a scalar value, we currently do not support merging ConditionalSelect uses into the intrinsic itself as a mask operation. So for patterns like Sve.AddAcross(Sve.ConditionalSelect(mask, vec, zero)), we only support emitting a SEL to first select the correct entries in the vector, and then passing this vector to AddAcross; #101973 tracks merging these two instructions into one for .NET 10.

When trying to merge ConditionalSelect operands of other intrinsics, the JIT did not skip this optimization for reduction intrinsics, breaking various invariants. This change ensures the JIT will not try to merge ConditionalSelect into reduction intrinsics, and adds relevant tests.

Regression

  • Yes
  • No

This was introduced with SVE support, which is new in .NET 9.

Testing

Added regression tests, and expanded existing SVE testing to cover more ConditionalSelect cases.

Risk

Low. This is specific to SVE support, which is experimental in .NET 9.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 11, 2024
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@amanasifkhalid
Copy link
Member Author

@kunalspathak PTAL, I had to open this manually due to merge conflicts around the ordering of HW_Flags in hwintrinsic.h. Also, since #107581 was backported before this one, lowerarmarch.cpp had trivial merge conflicts.

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!

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we can merge when ready

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 12, 2024
@amanasifkhalid
Copy link
Member Author

@jeffschwMSFT could you merge this when you have a moment, please? #107725 needs to be merged on top of this PR. Thanks!

@jeffschwMSFT jeffschwMSFT merged commit 165f846 into dotnet:release/9.0 Sep 12, 2024
10 checks passed
@amanasifkhalid amanasifkhalid deleted the backport-107207 branch September 12, 2024 16:30
@github-actions github-actions bot locked and limited conversation to collaborators Oct 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants