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 IF_SVE_AF_3A to IF_SVE_AQ_3A #95337

Merged
merged 5 commits into from
Nov 29, 2023

Conversation

a74nh
Copy link
Contributor

@a74nh a74nh commented Nov 28, 2023

Next set of encodings to 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 Nov 28, 2023
@ghost
Copy link

ghost commented Nov 28, 2023

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

Issue Details

Next set of encodings to contribute towards #94549

Author: a74nh
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@a74nh
Copy link
Contributor Author

a74nh commented Nov 28, 2023

Some of the encodings aren't yet supported by capstone. These are all quadword instructions are are not available in N2. These are untested, so the codegen code has been commented out. I've left in the code emit code for these as it should be mostly correct - but it is the job of whoever enables the codegen code to fully test it.

@a74nh
Copy link
Contributor Author

a74nh commented Nov 28, 2023

The number of instructions in a group does not increase the time it takes to code that group. Instead, it is the uniqueness of the encoding that takes the time.

This PR took me probably 4 or so hours work. There are 8 groups. So, 30mins per group. I timed one group, and that was about right. I suspect we still have some awkward encoding oddities still to come. Maybe 1 hour per group is a safer estimate going forwards.

@a74nh
Copy link
Contributor Author

a74nh commented Nov 28, 2023

@kunalspathak
Copy link
Member

Next set of encodings to contribute towards #94549

I don't see these 2 encodings...are you planning to add them later?

SVE_AB_3B
SVE_HL_3B

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 LGTM. added some suggestion on reducing the TP impact.


// IF_SVE_AG_3A
// These are not yet supported by capstone.
// theEmitter->emitIns_R_R_R(INS_sve_andqv, EA_1BYTE, REG_V4, REG_P4, REG_V4, INS_OPTS_SCALABLE_B_TO_SIMD_VECTOR);
Copy link
Member

Choose a reason for hiding this comment

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

can these be verified on linux gdb/lldb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like these are not yet supported by gdb and binutils.

I've added ALL_ARM64_EMITTER_UNIT_TESTS_SVE_UNSUPPORTED in the codegen tests, plus I've added unreached() in the emit_R_R_R() for those instructions. That should prevent them from being used.

src/coreclr/jit/emitarm64.cpp Show resolved Hide resolved
src/coreclr/jit/emitarm64.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/instr.h Show resolved Hide resolved
src/coreclr/jit/emitarm64.cpp Show resolved Hide resolved
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Nov 28, 2023
@a74nh
Copy link
Contributor Author

a74nh commented Nov 28, 2023

Next set of encodings to contribute towards #94549

I don't see these 2 encodings...are you planning to add them later?

SVE_AB_3B
SVE_HL_3B

Both of these groups only contain instructions not in capstone. I decided to move onto the next set of groups for now. I'll go back to them after this one.

Change-Id: I068d2761af40d22f8850e64d136a81275ce4ca5f
Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

A couple nits/minor comments

src/coreclr/jit/emitarm64.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/emitarm64.cpp Show resolved Hide resolved
case IF_SVE_AI_3A: // ........xx...... ...gggnnnnnddddd -- SVE integer add reduction (predicated)
case IF_SVE_AJ_3A: // ........xx...... ...gggnnnnnddddd -- SVE integer add reduction (quadwords)
case IF_SVE_AK_3A: // ........xx...... ...gggnnnnnddddd -- SVE integer min/max reduction (predicated)
case IF_SVE_AL_3A: // ........xx...... ...gggnnnnnddddd -- SVE integer min/max reduction (quadwords)
Copy link
Member

Choose a reason for hiding this comment

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

Should we mix the 'n' and 'm' patterns in the same "case"? I suppose it's assuming that 'idReg3()' is either 'm' or 'n', depending on the format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, idReg3() is the same in both.

I wasn't sure whether to keep them the same either - but, because it's the same code I went with the one block.

emitDispSveReg(id->idReg3(), id->idInsOpt(), false); // mmmmm
break;

// <Vd>.<T>, <Pg>, <Zn>.<Tb>
Copy link
Member

Choose a reason for hiding this comment

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

This comment is very helpful. Maybe we should add them to all case "sets"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've already done it for all the other SVE instructions done so far. I'm not planning on going back over NEON etc. :)

@a74nh a74nh marked this pull request as ready for review November 29, 2023 15:37
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

@@ -5437,6 +5437,7 @@ void CodeGen::genProfilingLeaveCallback(unsigned helper)
// #define ALL_ARM64_EMITTER_UNIT_TESTS_GENERAL
// #define ALL_ARM64_EMITTER_UNIT_TESTS_ADVSIMD
// #define ALL_ARM64_EMITTER_UNIT_TESTS_SVE
// #define ALL_ARM64_EMITTER_UNIT_TESTS_SVE_UNSUPPORTED
Copy link
Member

Choose a reason for hiding this comment

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

not sure if it is worth adding a comment in next PR why they are not supported. "The encodings of the instructions cannot be verified currently, so are added under this flag"

case INS_sve_andqv:
case INS_sve_eorqv:
case INS_sve_orqv:
unreached(); // TODO-SVE: Not yet supported.
Copy link
Member

Choose a reason for hiding this comment

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

likewise here.

Copy link
Member

Choose a reason for hiding this comment

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

can any of these instructions block us from the basic scenario that we are thinking of enabling?

@kunalspathak kunalspathak merged commit 8cb3c61 into dotnet:main Nov 29, 2023
129 checks passed
@a74nh a74nh deleted the IF_SVE_AF_3A_github branch November 30, 2023 10:32
@BruceForstall BruceForstall added the arm-sve Work related to arm64 SVE/SVE2 support label Dec 7, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 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