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

riscv64: Add VecALUImm instruction format #6325

Merged
merged 6 commits into from
May 4, 2023

Conversation

afonso360
Copy link
Contributor

👋 Hey,

This PR adds a new vector instruction format Vector+Imm. The encoding is exactly the same as v*.vv but the second register is instead a 5 bit field. This immediate is signed for most ops, but unsigned for some.

@afonso360 afonso360 requested a review from a team as a code owner May 2, 2023 13:54
@afonso360 afonso360 requested review from elliottt and removed request for a team May 2, 2023 13:54
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label May 2, 2023
cranelift/codegen/src/isa/riscv64/lower/isle.rs Outdated Show resolved Hide resolved
pub fn funct6(&self) -> u32 {
// See: https://github.com/riscv/riscv-v-spec/blob/master/inst-table.adoc
match self {
VecAluOpRRImm5::Vadd => 0b000000,
Copy link
Member

Choose a reason for hiding this comment

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

I don't know much about RISC-V, but should this AluOp be different than VecAluOpRRR or the same? The single data point of Vadd has the encodings the same, but I'm not sure if the pattern holds up elsewhere.

Copy link
Contributor Author

@afonso360 afonso360 May 3, 2023

Choose a reason for hiding this comment

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

Well, It is the same, but not all opcodes work on both formats. vadd works for both, but vsub does not have a vsub.vi format. It would be nice if we could share the funct6 mapping somehow, since it is exactly the same on both for the cases that do exist, I'm just not sure how.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok makes sense, in that case sounds good to me to have a minor amount of duplication 👍

cranelift/codegen/src/isa/riscv64/inst/encode.rs Outdated Show resolved Hide resolved
Comment on lines 264 to 265
// OPIVI
0b011
Copy link
Member

Choose a reason for hiding this comment

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

My curiosity has been piqued if you're willing to oblige -- according to this doc I see a bunch of V/X/I columns but I'm not sure how that translates to the encoding. Is there a different document that explains how these columns relate to the encoding?

Copy link
Contributor Author

@afonso360 afonso360 May 3, 2023

Choose a reason for hiding this comment

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

That table does not seem to be explained anywhere, which is weird. The closest thing is 10. Vector Arithmetic Instruction Formats which only really explains the format, but not the encoding in that table. The way I understand it is the following.

There are 3 columns, which correspond to the 3 columns for the minor opcodes at the top (i.e. OPIVV, OPIVX, OPMVX, etc..). The minor opcode corresponds to the funct3 field. (The exact encoding is specified here)

The minor opcodes correspond to the operands that instruction takes. OPIVV is for Vector/Vector instructions (i.e. vadd.vv), OPIVX is for Vector/X Register instructions (i.e. vadd.vx) and OPIVI is for Vector/Immediate instructions (i.e. vadd.vi) which is what is implemented in this PR.

I'm only planning on having 2 instruction formats, since OP*VV,OP*VX and OPFVF can share the same instruction format in cranelift. Just with an assert that we are using the correct regclass. OPIVI is the only special one here.

The big table displays the funct6 field. Take for example the vrsub instruction. It is in the leftmost column, and only has X and I formats. So we know that it only takes the minor opcode formats of OPIVX and OPIVI. vrsub itself has a funct6 of 0b000011 with both of those minor opcodes.

There are additional encoding spaces, at the bottom, but my understanding is that these are all OPIVI instructions and that encoding gets put in the immediate field (not 100% sure on this yet).

Copy link
Contributor Author

@afonso360 afonso360 May 3, 2023

Choose a reason for hiding this comment

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

I've also now put these into a VecOpCategory struct so that their encoding is somewhat centralized.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to explain! That all sounds good to me and everything looks good to me as well 👍

@afonso360 afonso360 added this pull request to the merge queue May 4, 2023
Merged via the queue into bytecodealliance:main with commit 6938a02 May 4, 2023
@afonso360 afonso360 deleted the riscv-vec-imm branch May 4, 2023 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants