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

Add Arm64 encodings for SVE_DD_2A & SVE_DG_2A #97446

Merged
merged 2 commits into from
Jan 25, 2024

Conversation

SwapnilGaikwad
Copy link
Contributor

These groups emit predicated version of pfirst, rdffr & rdffrs instructions.

This clr output matches the one from capstone.

...
pfirst p0.b, p15, p0.b
rdffr p10.b, p15/z
rdffrs p7.b, p14/z
...

Contribute towards #94549.

@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 Jan 24, 2024
@ghost
Copy link

ghost commented Jan 24, 2024

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

Issue Details

These groups emit predicated version of pfirst, rdffr & rdffrs instructions.

This clr output matches the one from capstone.

...
pfirst p0.b, p15, p0.b
rdffr p10.b, p15/z
rdffrs p7.b, p14/z
...

Contribute towards #94549.

Author: SwapnilGaikwad
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SwapnilGaikwad SwapnilGaikwad marked this pull request as ready for review January 24, 2024 14:54
@SwapnilGaikwad
Copy link
Contributor Author

@a74nh @kunalspathak @dotnet/arm64-contrib

case IF_SVE_DF_2A:
case IF_SVE_GS_3A:
case IF_SVE_HJ_3A:
case IF_SVE_IY_4A:
return PREDICATE_NONE;

case IF_SVE_DD_2A:
Copy link
Member

Choose a reason for hiding this comment

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

This should be always PREDICATE_NONE for DD_2A, right? https://docsmirror.github.io/A64/2023-06/pfirst_p_p_p.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs PREDICATE_NONE for the second reg only else we will miss the element type on remaining predicate registers. It would cause the following diff with capstone.

- pfirst p0.b, p15, p0.b
+ pfirst p0, p15, p0

Copy link
Contributor

@TIHan TIHan Jan 25, 2024

Choose a reason for hiding this comment

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

@kunalspathak , this was one of the reasons why insGetPredicateType was originally called something like insGetReg2PredicateType.

Because pfirst has two predicate registers, insGetPredicateType is ambiguous of what it should return.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just now noticed the regPos was added. That seems to clear up any ambiguity.

Copy link
Member

Choose a reason for hiding this comment

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

right...having regPos is better.

case IF_SVE_CX_4A:
case IF_SVE_CX_4A_A:
case IF_SVE_CY_3A:
case IF_SVE_CY_3B:
case IF_SVE_DG_2A:
Copy link
Member

Choose a reason for hiding this comment

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

From https://docsmirror.github.io/A64/2023-06/rdffr_p_p_f.html, this should be:

case IF_SVE_DG_2A:
   return ((regpos == 2) ? PREDICATE_ZERO: PREDICATE_NONE);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it would avoid asserts and make IF_SVE_DG_2A a separate case for groups that have only two registers and those are predicate registers. The other groups also have only two predicate registers but they have non-predicated registers as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, and we need PREDICATE_SIZED for reg1 otherwise we won't get the element type in <Pd>.B.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 24, 2024
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 25, 2024
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!

@kunalspathak kunalspathak merged commit c280006 into dotnet:main Jan 25, 2024
126 of 129 checks passed
Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

LGTM, pretty straightforward.

@SwapnilGaikwad SwapnilGaikwad deleted the github-sve-dd-dg branch January 26, 2024 09:54
@github-actions github-actions bot locked and limited conversation to collaborators Feb 26, 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