Skip to content

Conversation

@kunalspathak
Copy link
Contributor

@kunalspathak kunalspathak commented Jul 1, 2024

For APIs does not take mask as 1st parameter, but generates only predicated instructions like Abs, we wrap it in CndSel(AllTrue, Operation(), Zero). If user however, writes a code explicitly using CndSel operation, we would skip wrapping it, because we were already in that form. However, this caused issues when such embedded mask operation was used in mergeValue operation, something like CndSel(mask, trueValue, Operation()).

To solve this, now we will wrap the operation, regardless of if it is inside CndSel or not. For example,

  1. CndSel(mask, Operation(), mergeValue) => CndSel(mask, CndSel(AllTrue, Operation(), Zero) mergeValue)
  2. CndSel(mask, trueValue, Operation()) => CndSel(mask, trueValue, CndSel(AllTrue, Operation(), Zero))

One of these forms will then get handled in new lowering method to handle CndSel nodes in general to do the right operation.

As part of this, I have updated all the test templates to pass Operation() for mergeValue.

TODO:

  • Update all the test templates
  • Add validation for CndSel(mask, Operation(), Operation()) Edit: For now, will add this as a separate pass.

All tests are passing. Results here.

Thanks to @tannergooding for spending time with me for coming up with the design.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 1, 2024
@dotnet-policy-service
Copy link
Contributor

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

@kunalspathak kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label Jul 1, 2024
@kunalspathak
Copy link
Contributor Author

@dotnet/arm64-contrib

@kunalspathak kunalspathak marked this pull request as ready for review July 3, 2024 07:59
@kunalspathak
Copy link
Contributor Author

@dotnet/arm64-contrib

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.

Test template changes LGTM, assuming SVE tests still pass. Thanks!

@kunalspathak
Copy link
Contributor Author

Test template changes LGTM, assuming SVE tests still pass. Thanks!

It does. Here are the results.

@kunalspathak kunalspathak merged commit a91700f into dotnet:main Jul 3, 2024
@kunalspathak kunalspathak deleted the cndsel branch July 3, 2024 22:10
@github-actions github-actions bot locked and limited conversation to collaborators Aug 3, 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 arm-sve Work related to arm64 SVE/SVE2 support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants