[RISCV] Implement the MC layer support of P extension

[Jim]

Summary

Contribute Andes's the MC layer support of P extension.

[achieveartificialintelligence]

This comment was removed by achieveartificialintelligence.

[asb]

Hi Jim, thanks for the contribution. I saw you were on the review thread for D94579 which also aims to implement MC layer support for the P extension - could you please comment on the difference between these two patches as you see it?

[Jim]

In D95588#2527882, @asb wrote:

Hi Jim, thanks for the contribution. I saw you were on the review thread for D94579 which also aims to implement MC layer support for the P extension - could you please comment on the difference between these two patches as you see it?

Hi,

I listed extra works in this implementation

1. Register class (GPRP) have vector types for 'P' instruction.
2. GPRPair is a special register class with even/odd pair of registers for RV32.

[jrtc27]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

967

Don't need a blank line above, and I'd put GPR stuff before FPR stuff.

982

I do not like the side-effecting function. Please make it look like the FPR code, which probably means splitting out the "is GPR pair" part from convertGPRToGPRPair (i.e. checking if it's an even register number). Also, seeing MCK_GPRPair for non-RV32 should always be an error (assertion failure), surely, and not just something to ignore?

llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

457

You should not need both namespaces, only one of them. I'd put the weirdo register pair instructions in a namespace and leave the normal non-pair RV64 versions in the standard namespace.

llvm/lib/Target/RISCV/RISCVInstrFormats.td

130

As I said in the other review, this goes against what the RISCV spec said it would do, as it's using an encoding it proposed reserved for >32-bit instructions. I'm not a fan of merging this until it's confirmed that this is the way things are definitely going to go, as it may affect downstreams that use the >32-bit encoding space to steer clear of conflicts (like hwacha did), but maybe there aren't any for us?

llvm/lib/Target/RISCV/RISCVInstrInfoP.td

53

Weird spacing

75

Style has been to put such constraints in the body rather than a let (see A and C).

118

That doesn't look like a uimm6 to me

llvm/lib/Target/RISCV/RISCVRegisterInfo.td

69

This assumes RV32, and is not clear it applies to register pairs

200

Needs a VT suffix, but it's not clear from the name what exactly this is

llvm/lib/Target/RISCV/RISCVSchedRocket.td

19 ↗

(On Diff #319756)

I've never understood why this isn't a SupportedFeatures field given processors don't grow new features but compilers do...

llvm/test/MC/RISCV/rvp/rv32p-invalid.s

2

You don't need rvXXp- prefixes on your files if they're inside rvp. Just use rvXX- if you need to specify XLEN.

llvm/test/MC/RISCV/rvp/rv64p-invalid.s

2

This is almost the same file as rv32p-invalid.s. Merge the common parts.

[asb]

Per our discussion in the RISC-V community call today, PLCT Lab + Andes are going to reach out to each other to try to coordinate these MC layer and future codegen patches (thanks!).

[Jim]

Address @jrtc27 's comments

[Jim]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

982

Inst with MCK_GPRPair version is still trying to match operand for non-RV32. It would be a error due to missing features later.

llvm/lib/Target/RISCV/RISCVInstrInfoP.td

118

Do you mean that is not the same as RVPShiftI5 with uimm5. It is not uimm6 here.

llvm/lib/Target/RISCV/RISCVRegisterInfo.td

200

Add suffix VT. Do you have any suggestion for this? Thanks.

llvm/test/MC/RISCV/rvp/rv32p-invalid.s

2

The range for immediate is difference between RV32 and RV64.

llvm/test/MC/RISCV/rvp/rv64p-invalid.s

2

The immediate range is difference between RV32 and RV64.

[Jim]

XLenI32VT has i32 on RV32.

[jrtc27]

llvm/lib/Target/RISCV/RISCVInstrInfoP.td

118

Then don't call it RVPShiftI6

llvm/lib/Target/RISCV/RISCVRegisterInfo.td

200

XLenVEI8VT / VXLenEI8VT / XVEI8VT / PVEI8VT / VPEI8VT all come to mind, maybe others have better ideas, but I think it's important to include the fact that it's a vector in the name otherwise it just looks like a weird way to say i8.

llvm/test/MC/RISCV/rvp/64-bit.s

24 ↗

(On Diff #332574)

Please put CHECK lines above not below

[craig.topper]

llvm/lib/Target/RISCV/RISCVRegisterInfo.td

209

Why do we need a special register class? If the size, alignment, spill size are the same, why can't we just add the types to the regular GPR class?

[Jim]

Merge https://reviews.llvm.org/D95589 and https://reviews.llvm.org/D95590 into this patch.

Implement P's sub-extensions Zpn, Zpsfoperand and Zprvsfextra.

[Jim]

llvm/lib/Target/RISCV/RISCVRegisterInfo.td

209

I have tried to add new types to the regular GPR class before. But it show lots of Could not infer all types in pattern! error message on building. It have to add explicit type (XLenVT in most cases) to the existed codegen patterns which use GPR. I am not sure that is a good solution.

[craig.topper]

llvm/lib/Target/RISCV/RISCVRegisterInfo.td

209

It's unfortunate, but it probably is the right solution since it will eliminate a special case in the assembler.

[craig.topper]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

65

Is this line longer than 80 columns?

[Jim]

Use regular GPR class instead GPRP

[Jim]

Rebase