Skip to content

Add Arm64 encodings for IF_SVE_CY_3A and IF_SVE_CY_3B group #96992

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
Merged
merged 3 commits into from
Jan 17, 2024
Merged

Add Arm64 encodings for IF_SVE_CY_3A and IF_SVE_CY_3B group #96992

merged 3 commits into from
Jan 17, 2024

Conversation

SwapnilGaikwad
Copy link
Contributor

This group emits various compare instructions.

This clr output matches the one from capstone.

cmpeq p15.b, p0/z, z31.b, #8
cmpge p11.h, p7/z, z21.h, #1
cmpgt p10.s, p1/z, z18.s, #4
cmple p8.d, p6/z, z11.d, #15
cmplt p7.b, p2/z, z8.b, #-16
cmpne p0.h, p5/z, z0.h, #-14
cmphi p15.b, p7/z, z19.b, #0
cmphs p11.h, p1/z, z0.h, #0x24
cmplo p8.s, p5/z, z21.s, #0x40
cmpls p0.d, p3/z, z9.d, #0x7F

Contribute towards #94549.

@ghost ghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member labels Jan 15, 2024
@ghost
Copy link

ghost commented Jan 15, 2024

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, p0/z, z31.b, #8
cmpge p11.h, p7/z, z21.h, #1
cmpgt p10.s, p1/z, z18.s, #4
cmple p8.d, p6/z, z11.d, #15
cmplt p7.b, p2/z, z8.b, #-16
cmpne p0.h, p5/z, z0.h, #-14
cmphi p15.b, p7/z, z19.b, #0
cmphs p11.h, p1/z, z0.h, #0x24
cmplo p8.s, p5/z, z21.s, #0x40
cmpls p0.d, p3/z, z9.d, #0x7F

Contribute towards #94549.

Author: SwapnilGaikwad
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

This was referenced Jan 15, 2024
Closed
Closed
@a74nh
Copy link
Contributor

a74nh commented Jan 17, 2024

@kunalspathak

a74nh
a74nh approved these changes Jan 17, 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.

Everything else LGTM.

src/coreclr/jit/emitarm64.cpp
case IF_SVE_CY_3A: // ........xx.iiiii ...gggnnnnn.DDDD -- SVE integer compare with signed immediate
case IF_SVE_CY_3B: // ........xx.iiiii ii.gggnnnnn.DDDD -- SVE integer compare with unsigned immediate
emitDispPredicateReg(id->idReg1(), PREDICATE_SIZED, id->idInsOpt(), true); // DDDD
emitDispPredicateReg(id->idReg2(), PREDICATE_ZERO, id->idInsOpt(), true); // ggg
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we should be using insGetPredicateType(fmt) instead of PREDICATE_ZERO.

But... on the previous line we would still need to do PREDICATE_SIZED.
We could extend insGetPredicateType() to have a an extra arg (reg position), which is ignored for most instructions.

Alternatively, we keep this PR as it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, we keep this PR as it is.

I agree. But we should have a follow up PR that calls insGetPredicateType() instead of hard-coding even at other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. But we should have a follow up PR that calls insGetPredicateType() instead of hard-coding even at other places.

Done: #97142

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

@dotnet/arm64-contrib

@kunalspathak kunalspathak mentioned this pull request Jan 17, 2024
Closed
kunalspathak
kunalspathak approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@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

src/coreclr/jit/emitarm64.cpp
assert(isLowPredicateRegister(id->idReg2())); // ggg
assert(isVectorRegister(id->idReg3())); // mmmmm
assert(isVectorRegister(id->idReg4())); // nnnnn
assert(isVectorRegister(id->idReg3())); // nnnnn
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch

src/coreclr/jit/emitarm64.cpp
case IF_SVE_CY_3A: // ........xx.iiiii ...gggnnnnn.DDDD -- SVE integer compare with signed immediate
case IF_SVE_CY_3B: // ........xx.iiiii ii.gggnnnnn.DDDD -- SVE integer compare with unsigned immediate
emitDispPredicateReg(id->idReg1(), PREDICATE_SIZED, id->idInsOpt(), true); // DDDD
emitDispPredicateReg(id->idReg2(), PREDICATE_ZERO, id->idInsOpt(), true); // ggg
Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, we keep this PR as it is.

I agree. But we should have a follow up PR that calls insGetPredicateType() instead of hard-coding even at other places.

This was referenced Jan 17, 2024
Open
Closed
@kunalspathak kunalspathak merged commit 08cff56 into dotnet:main Jan 17, 2024
@a74nh a74nh mentioned this pull request Jan 18, 2024
Merged
@SwapnilGaikwad SwapnilGaikwad deleted the github-sve-cy-3a branch January 22, 2024 10:17
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@SwapnilGaikwad @kunalspathak @tmds
Add Arm64 encodings for IF_SVE_CY_3A and IF_SVE_CY_3B group (dotnet#9…
b96130a 
…6992)

* Add Arm64 encodings for IF_SVE_CY_3A and IF_SVE_CY_3B group

* Fix function declaration

---------

Co-authored-by: Kunal Pathak <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@a74nh a74nh a74nh approved these changes

@kunalspathak kunalspathak kunalspathak approved these changes

Reviewers whose approvals may not affect merge requirements

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 @a74nh @kunalspathak