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

Add Arm64 encodings for SVE IF_SVE_CX_4A_A to IF_SVE_HT_4A group #96214

Merged
merged 9 commits into from
Jan 12, 2024
Merged

Add Arm64 encodings for SVE IF_SVE_CX_4A_A to IF_SVE_HT_4A group #96214

merged 9 commits into from
Jan 12, 2024
+189 −23

Conversation

SwapnilGaikwad
Copy link
Contributor

@SwapnilGaikwad SwapnilGaikwad commented Dec 20, 2023 •
edited
Loading

This group emits various compare instructions.

This clr output matches the one from capstone.

...
cmpeq p15.b, p7/z, z31.b, z3.d
cmpge p14.h, p6/z, z21.h, z13.d
cmpgt p13.s, p5/z, z11.s, z23.d
cmphi p12.b, p4/z, z1.b, z31.d
cmphs p11.h, p3/z, z0.h, z30.d
cmpge p4.s, p2/z, z0.s, z10.d
cmphi p3.b, p1/z, z1.b, z20.d
cmphs p2.h, p0/z, z2.h, z30.d
cmpgt p1.s, p7/z, z8.s, z24.d
cmpne p0.b, p0/z, z14.b, z28.d
match p15.b, p0/z, z21.b, z0.b
nmatch p0.h, p7/z, z11.h, z31.h
facge p0.h, p0/z, z10.h, z31.h
facgt p15.s, p1/z, z20.s, z21.s
facge p1.d, p2/z, z11.d, z0.d
facgt p14.h, p3/z, z1.h, z30.h
fcmeq p2.s, p4/z, z28.s, z8.s
fcmge p13.d, p5/z, z8.d, z18.d
fcmgt p3.h, p6/z, z18.h, z28.h
fcmge p12.s, p7/z, z30.s, z1.s
fcmgt p4.d, p0/z, z0.d, z11.d
fcmne p11.h, p1/z, z21.h, z10.h
fcmuo p5.s, p2/z, z31.s, z20.s
...

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 Dec 20, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 20, 2023
@ghost
Copy link

ghost commented Dec 20, 2023

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

Issue Details

This group emits various compare instructions.

This clr output matches the one from capstone.

...
cmpeq p15.b, p7/z, z31.b, z3.d
cmpge p14.h, p6/z, z21.h, z13.d
cmpgt p13.s, p5/z, z11.s, z23.d
cmphi p12.b, p4/z, z1.b, z31.d
cmphs p11.h, p3/z, z0.h, z30.d
cmpge p4.s, p2/z, z0.s, z10.d
cmphi p3.b, p1/z, z1.b, z20.d
cmphs p2.h, p0/z, z2.h, z30.d
cmpgt p1.s, p7/z, z8.s, z24.d
cmpne p0.b, p0/z, z14.b, z28.d
...

Contribute towards #94549.

Author: SwapnilGaikwad
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution
Milestone: -

@SwapnilGaikwad SwapnilGaikwad changed the title Add Arm64 encodings for SVE IF_SVE_CQ_4A_A group Add Arm64 encodings for SVE IF_SVE_CQ_4A_A & IF_SVE_GE_4A group Dec 20, 2023
@build-analysis build-analysis bot mentioned this pull request Dec 21, 2023
Closed
@build-analysis build-analysis bot mentioned this pull request Jan 9, 2024
Closed
@SwapnilGaikwad SwapnilGaikwad changed the title Add Arm64 encodings for SVE IF_SVE_CQ_4A_A & IF_SVE_GE_4A group Add Arm64 encodings for SVE IF_SVE_CQ_4A_A to IF_SVE_HT_4A group Jan 10, 2024
a74nh
a74nh reviewed Jan 10, 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.

Otherwise, LGTM.

Also, @kunalspathak

src/coreclr/jit/codegenarm64test.cpp Outdated Show resolved Hide resolved
@kunalspathak kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label Jan 11, 2024
@build-analysis build-analysis bot mentioned this pull request Jan 12, 2024
Closed
a74nh
a74nh reviewed Jan 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.

LGTM except for the one comment.

src/coreclr/jit/emitarm64.cpp Outdated Show resolved Hide resolved
a74nh
a74nh approved these changes Jan 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.

LGTM

@kunalspathak kunalspathak changed the title Add Arm64 encodings for SVE IF_SVE_CQ_4A_A to IF_SVE_HT_4A group Add Arm64 encodings for SVE IF_SVE_CX_4A_A to IF_SVE_HT_4A group Jan 12, 2024
@kunalspathak kunalspathak mentioned this pull request Jan 12, 2024
Closed
kunalspathak
kunalspathak reviewed Jan 12, 2024
View reviewed changes
src/coreclr/jit/emitarm64.cpp
emitDispPredicateReg(id->idReg1(), PREDICATE_SIZED, id->idInsOpt(), true); // DDDD
emitDispPredicateReg(id->idReg2(), PREDICATE_ZERO, id->idInsOpt(), true); // ggg
emitDispSveReg(id->idReg3(), id->idInsOpt(), true); // mmmmm
emitDispSveReg(id->idReg4(), id->idInsOpt(), false); // nnnnn
emitDispSveReg(id->idReg4(), INS_OPTS_SCALABLE_D, false); // nnnnn
Copy link
Member

Choose a reason for hiding this comment

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

While passing INS_OPTS_SCALABLE_D looks fine for this instruction format, I was checking other formats where we directly pass the INS_OPTS and seems like for IF_SVE_EQ_3A, we should have a method that is reverse of optWidenSveElemsizeArrangement(), which will basically lower H->B, S->H and D->S instead of manipulating the idOpts() this way.

runtime/src/coreclr/jit/emitarm64.cpp

Lines 18752 to 18755 in b498582

case IF_SVE_EQ_3A: // ........xx...... ...gggnnnnnddddd -- SVE2 integer pairwise add and accumulate long
emitDispSveReg(id->idReg1(), id->idInsOpt(), true); // ddddd
emitDispLowPredicateReg(id->idReg2(), PREDICATE_MERGE, id->idInsOpt(), true); // ggg
emitDispSveReg(id->idReg3(), (insOpts)((unsigned)id->idInsOpt() - 1), false); // mmmmm

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, makes sense. I will piggyback this change on my next patch 👍

kunalspathak
kunalspathak approved these changes Jan 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

This was referenced Jan 12, 2024
Open
Closed
@kunalspathak
Copy link
Member

failures seems to known

@kunalspathak kunalspathak merged commit de1e529 into dotnet:main Jan 12, 2024
126 of 129 checks passed
@SwapnilGaikwad SwapnilGaikwad deleted the github-sve-cx_4a_a branch January 12, 2024 18:01
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@SwapnilGaikwad @tmds
Add Arm64 encodings for SVE IF_SVE_CX_4A_A to IF_SVE_HT_4A group (dot…
7b4937e 
…net#96214)

* Add SVE IF_SVE_CQ_4A_A group

* Fix format issues

* Add Arm64 encodings for IF_SVE_GE_4A group

* Fix build issue

* Add Arm64 encodings for case IF_SVE_HT_4A group

* Fix build and formatting

* Remove redundant asserts
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 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
No one assigned
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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SwapnilGaikwad @kunalspathak @a74nh