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

JIT ARM64-SVE2: Add IF_SVE_DQ_0A, IF_SVE_DR_1A, IF_SVE_DS_2A #95996

Merged
merged 8 commits into from
Dec 18, 2023
Merged

JIT ARM64-SVE2: Add IF_SVE_DQ_0A, IF_SVE_DR_1A, IF_SVE_DS_2A #95996

merged 8 commits into from
Dec 18, 2023

Conversation

amanasifkhalid
Copy link
Member

Part of #94549. This change implements the IF_SVE_DQ_0A, IF_SVE_DR_1A, and IF_SVE_DS_2A encodings. JIT disasm output:

setffr
wrffr   p0.b
ctermeq x0, x1
ctermeq w2, w3
ctermne x4, x5
ctermne w6, w7

cstool output:

00902C25  setffr
00902825  wrffr p0.b
0020E125  ctermeq       x0, x1
4020A325  ctermeq       w2, w3
9020E525  ctermne       x4, x5
D020A725  ctermne       w6, w7

cc @dotnet/arm64-contrib

@amanasifkhalid amanasifkhalid added the arm-sve Work related to arm64 SVE/SVE2 support label Dec 14, 2023
@ghost ghost assigned amanasifkhalid Dec 14, 2023
@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 Dec 14, 2023
@ghost
Copy link

ghost commented Dec 14, 2023

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

Issue Details

Part of #94549. This change implements the IF_SVE_DQ_0A, IF_SVE_DR_1A, and IF_SVE_DS_2A encodings. JIT disasm output:

setffr
wrffr   p0.b
ctermeq x0, x1
ctermeq w2, w3
ctermne x4, x5
ctermne w6, w7

cstool output:

00902C25  setffr
00902825  wrffr p0.b
0020E125  ctermeq       x0, x1
4020A325  ctermeq       w2, w3
9020E525  ctermne       x4, x5
D020A725  ctermne       w6, w7

cc @dotnet/arm64-contrib

Author: amanasifkhalid
Assignees: amanasifkhalid
Labels:

area-CodeGen-coreclr, arch-arm64-sve
Milestone: -

@amanasifkhalid amanasifkhalid mentioned this pull request Dec 14, 2023
Closed
@BruceForstall
Copy link
Member

cc @a74nh

a74nh
a74nh reviewed Dec 15, 2023
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.

LGTM, just needs the extra comments.

src/coreclr/jit/emitarm64.cpp
dst += emitOutput_Instr(dst, code);
break;

case IF_SVE_DS_2A: // .........x.mmmmm ......nnnnn..... -- SVE conditionally terminate scalars
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add the display encoding (just copy/paste from the codegen tests) above the case:
// <R><n>, <R><m>

This makes is easy to group together the encoding groups with the same displays.

Same for the other two groups (SetFFR would be // none or something similar)

@a74nh
Copy link
Contributor

a74nh commented Dec 15, 2023

FYI, This PR will need rebasing - #96005 moved the codegen tests to a new file.

@amanasifkhalid
Copy link
Member Author

@a74nh thank you for the review! I've added your suggested comments and merged in the unit test changes.

@a74nh
Copy link
Contributor

a74nh commented Dec 15, 2023

@a74nh thank you for the review! I've added your suggested comments and merged in the unit test changes.

Thanks. All LGTM now.

kunalspathak
kunalspathak requested changes Dec 18, 2023
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.

Changes looks good. One minor feedback.

src/coreclr/jit/emitarm64.cpp Outdated
* Add a SETFFR instruction: initialize first-fault register to all true.
*/

void emitter::emitInsSve_SetFFR()
Copy link
Member

Choose a reason for hiding this comment

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

We should probably reuse emitIns_I() for it. It encodes the instructions that don't take registers.

src/coreclr/jit/emitarm64.h Outdated
@@ -340,6 +340,9 @@ static code_t insEncodeReg_V_21_to_17(regNumber reg);
// Return an encoding for the specified 'R' register used in '21' thru '17' position.
static code_t insEncodeReg_R_21_to_17(regNumber reg);

// Return an encoding for the specified 'R' register used in '20' thru '16' position.
static code_t insEncodeReg_R_20_to_16(regNumber reg);
Copy link
Member

Choose a reason for hiding this comment

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

I have fixed this in #96082.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for letting me know. It looks like your PR will get merged in pretty soon, so I'll wait to merge your changes into mine.

Copy link
Member

Choose a reason for hiding this comment

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

@amanasifkhalid - do you mind addressing #96082 (comment) in your PR, since you will have another round of CI run anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing, I'll update the assert.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 18, 2023
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 18, 2023
@amanasifkhalid
Copy link
Member Author

@kunalspathak thank you for the review! I've consolidated the setffr logic into emitIns_I. I'll wait for #96082 to be merged in, and then merge your changes to the register encoding methods into mine.

kunalspathak
kunalspathak approved these changes Dec 18, 2023
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

@kunalspathak kunalspathak mentioned this pull request Dec 18, 2023
Merged
@amanasifkhalid
Copy link
Member Author

@kunalspathak I've updated the assert, PTAL

kunalspathak
kunalspathak approved these changes Dec 18, 2023
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

@amanasifkhalid amanasifkhalid merged commit 7693465 into dotnet:main Dec 18, 2023
13 of 95 checks passed
@amanasifkhalid amanasifkhalid mentioned this pull request Dec 18, 2023
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Jan 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kunalspathak kunalspathak kunalspathak approved these changes

@a74nh a74nh a74nh 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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amanasifkhalid @BruceForstall @a74nh @kunalspathak