This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] add the MC layer support of P extension
AbandonedPublic

Authored by achieveartificialintelligence on Jan 12 2021, 7:38 PM.

Details

Summary

This patch added the MC layer support of P extension.

Authored-by: Shao-Ce Sun
Co-Authored-by: StephenFan

Diff Detail

Event Timeline

achieveartificialintelligence requested review of this revision.Jan 12 2021, 7:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 7:38 PM

Please make sure there are tests for invalid instructions (especially checking you have the immediate ranges and predicates correct).

I have not looked at the draft P spec so these are all shallow comments from glancing over the diff.

llvm/lib/Target/RISCV/RISCV.td
188

"SIMD Instructions" would be most consistent with what's already here (though the V-related extensions have names that aren't so consistent with that) IMO, though the P working group should probably come up with a better name than SIMD in order to distinguish themselves from V (with P being traditional vectorisation and V being new-fangled scalable vectorisation).

llvm/lib/Target/RISCV/RISCVInstrFormats.td
138

Aren't all major opcodes of the form 0bXX11111 reserved for >32-bit encodings (with 0b1111111 in particular being >=80-bit)? This is Table 24.1 Chapter 24 of v20191214, and that's still present in the latest LaTeX source in the riscv-isa-manual repo.

llvm/lib/Target/RISCV/RISCVInstrInfo.td
1263

P comes between B and V in the canonical extension order

llvm/lib/Target/RISCV/RISCVInstrInfoP.td
391

If this is the same situation as the base instruction set's bit shift instructions use a uimmlog2xlen like those rather than having two different instructions (and sets of patterns)

395

Capital U

kito-cheng added inline comments.Jan 12 2021, 10:42 PM
llvm/lib/Target/RISCV/RISCV.td
188

Maybe we could using the same term in the ISA manual: “P” Standard Extension for Packed-SIMD Instructions

Jim added a comment.Jan 13 2021, 2:44 AM

It seems lack of invalid operand handling for new added operand (uimm3, uimm4) in AsmParser/RISCVAsmParser.cpp:MatchAndEmitInstruction.

llvm/lib/Target/RISCV/RISCVInstrInfoP.td
26

This should be (ins GPR:$rs1)

28

This should be "let Inst{24-20} = funct5"

154

KDMABB's rd register is also a input operand. It should create another class for this kind instruction.

507

Could you refer RISCVInstrInfoV.td to add comment between operand, class declaration and instruction definition.
For more readable, could you add category name before instructions.

llvm/lib/Target/RISCV/RISCVSubtarget.h
56

Put it between B and V in the canonical extension order

123

Put it between B and V in the canonical extension order

Jim added inline comments.Jan 13 2021, 2:55 AM
llvm/lib/Target/RISCV/RISCVInstrInfoP.td
115

In RV32, ADD64's input operand is even/odd pair of registers.

Thanks Everyone for all the comments. We will update the code later.

lenary resigned from this revision.Jan 14 2021, 9:25 AM
achieveartificialintelligence marked 12 inline comments as done.
asb added a reviewer: asb.Jan 28 2021, 5:00 AM

Thanks for submitting this. It doesn't apply against current HEAD, could you please rebase?

One minor request before reviewing in detail - could you please confirm the version of the spec this is meant to implement? Is it 0.9.1 at https://github.com/riscv/riscv-p-spec/blob/master/P-ext-proposal.adoc#revision-history ? It would also be good to indicate this version number in RISCVInstrInfoP.td just as we do for RISCVInstrInfoB.td.

Jim added a comment.Jan 28 2021, 5:27 PM

The registers used in most 'P' instruction should have v4i8 and v2i16 types for RV32 or v8i8 and v4i16 for RV64.

llvm/lib/Target/RISCV/RISCVInstrInfoP.td
431

The imm range for INSB_32 and INSB_64 is log of number of bytes (RV32 is 4, RV64 is 8).
It can be combined as one instruction.

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
183 ↗(On Diff #317779)

It should combine (X10, X11), (X12, X13), .... together.
Not just represented as first register.

asb added a comment.EditedFeb 4 2021, 8:28 AM

Per our discussion in the RISC-V community call today, this patch is intended as more of a "request for comment" at this stage given the P extension encoding issues, and PLCT Lab + Andes are going to reach out to each other to try to coordinate these MC layer and future codegen patches (thanks!).