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

Arm64/SVE: Add insEncodeReg* methods #95105

Merged
merged 4 commits into from
Nov 23, 2023

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented Nov 22, 2023

Add bunch of insEncodeReg* methods because not all instructions have registers position at same position. See details at
#94549 (comment). I have updated the emitOutputInstr_sve.cpp.txt in #94549 to call these methods.

Contributes to #94549

@ghost ghost assigned kunalspathak Nov 22, 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 Nov 22, 2023
@ghost
Copy link

ghost commented Nov 22, 2023

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

Issue Details

Add bunch of insEncodeReg* methods because not all instructions have registers position at same position. See details at
#94549 (comment). I have updated the emitOutputInstr_sve.cpp.txt in #94549 to call these methods.

Author: kunalspathak
Assignees: kunalspathak
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak kunalspathak changed the title Add insEncodeReg* methods Arm64/SVE: Add insEncodeReg* methods Nov 22, 2023
@kunalspathak kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label Nov 22, 2023
@kunalspathak
Copy link
Member Author


/*static*/ emitter::code_t emitter::insEncodeReg_V_9_to_5(regNumber reg)
{
assert(isVectorRegister(reg));
Copy link
Contributor

Choose a reason for hiding this comment

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

I want to change this to insEncodeReg_V_9_to_5(regNumber reg, int lowbit, int highbit)
But your way is safer.

At the risk of making more hard to parse macros:

#define insEncodeFunc(highbit, lowbit) emitter::code_t emitter::insEncodeReg_V_#highbit_to_#lowbit(regNumber reg) /
{ /
    emitter::code_t ureg = (emitter::code_t)reg - (emitter::code_t)REG_V0; /
    assert((ureg >= 0) && (ureg <= 32)); /
    return ureg << 5; /
} /

insEncodeFunc(9, 5);
insEncodeFunc(12, 10);
etc

Copy link
Member Author

Choose a reason for hiding this comment

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

@BruceForstall - any preference? if we don't want to use macro, we can also convert in a function that takes lowbit as a parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

For some of the predicate ones, I've had to change them to assert(isLowPredicateRegister(reg)); because they only have 4 bits, as opposed to 5 bits.

So, maybe better leaving as functions.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer functions, even if they are quite duplicative. Makes for easier debugging. Sounds like @a74nh already found reasons the asserts would be different.

static code_t insEncodeReg_P_2_to_0(regNumber reg);

// Return an encoding for the specified register used in '19' thru '17' position.
static code_t insEncodeReg_V_19_to_17(regNumber reg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should all the V ones be Z?

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't introduced any "Z" terminology except for displaying the disassembly so kept it as insEncodeReg_V*. Even we use REG_V0, etc. Perhaps I should add aliases for "Z" registers and use it as well as rename methods whereever necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be Z, and introduce REG_Z0 etc to make the code easier to read.
Same with REG_P0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done for #95129

static code_t insEncodeReg_V_20_to_17(regNumber reg);

// Return an encoding for the specified register used in '9' thru '6' position.
static code_t insEncodeReg_V_9_to_6(regNumber reg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, you already have 9_to_5.
Confused as to exactly what this one is, as it is only 4 bits.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I search for insEncodeReg_V_9_to_6 in emitOutputInstr_sve.cpp, I see this:

case IF_SVE_HG_2A:   // ................ ......nnnn.ddddd -- SVE2 FP8 downconverts
   code = emitInsCodeSve(ins, fmt);
   code |= insEncodeReg_V_4_to_0(id->idReg10()); // ddddd
   code |= insEncodeReg_V_9_to_6(id->idReg20()); // nnnn
   dst += emitOutput_Instr(dst, code);
   break;
case IF_SVE_GA_2A:   // ............iiii ......nnnn.ddddd -- SME2 multi-vec shift narrow
   code = emitInsCodeSve(ins, fmt);
   code |= insEncodeReg_V_4_to_0(id->idReg10()); // ddddd
   code |= insEncodeReg_V_9_to_6(id->idReg20()); // nnnn
   code |= insEncodeImm(); // iiii
   dst += emitOutput_Instr(dst, code);
   break;

Then, searching for IF_SVE_HG_2A and IF_SVE_GA_2A in instrsarm64_sve.h`, I see this:

//    enum               name                     info                                              SVE_HG_2A                                    
INST1(bfcvtn,            "bfcvtn",                0,                       IF_SVE_HG_2A,            0x650A3800                                   )
    // BFCVTN  <Zd>.B, {<Zn1>.H-<Zn2>.H }                                                SVE_HG_2A           0110010100001010 001110nnnn0ddddd     650A 3800   

//    enum               name                     info                                              SVE_GA_2A                                    
INST1(sqrshrn,           "sqrshrn",               RSH,                     IF_SVE_GA_2A,            0x45B02800                                   )
    // SQRSHRN <Zd>.H, {<Zn1>.S-<Zn2>.S }, #<const>                                      SVE_GA_2A           010001011011iiii 001010nnnn0ddddd     45B0 2800  
image image

Most likely the SVE2 instructions takes only 4 bits for Z registers. Also it seems that https://docsmirror.github.io/A64/2023-06/sveindex.html doesn't have SVE2 instructions. I didn't find an entry of "bfcvtn" in there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, looks like this is similar to the instructions that only allow predicate registers 0 to 7.

src/coreclr/jit/emitarm64.h Outdated Show resolved Hide resolved
@kunalspathak kunalspathak merged commit 944c4aa into dotnet:main Nov 23, 2023
129 checks passed
@kunalspathak kunalspathak deleted the insEncodeReg branch November 23, 2023 01:13
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2023
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