Page MenuHomePhabricator

[RISCV] Implement the MC layer support of P extension
Needs ReviewPublic

Authored by Jim on Jan 27 2021, 9:27 PM.

Details

Summary

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

Diff Detail

Event Timeline

Jim created this revision.Jan 27 2021, 9:27 PM
Jim requested review of this revision.Jan 27 2021, 9:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2021, 9:27 PM
asb added a reviewer: asb.Jan 28 2021, 5:04 AM

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 added a comment.Jan 28 2021, 5:43 PM
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 added inline comments.Jan 28 2021, 6:09 PM
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 added a comment.Feb 4 2021, 8:29 AM

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 updated this revision to Diff 322058.Feb 8 2021, 2:28 AM

Address @jrtc27 's comments

Jim marked 4 inline comments as done.Feb 8 2021, 2:44 AM
Jim added inline comments.
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 updated this revision to Diff 332574.Mar 23 2021, 2:17 AM
Jim marked an inline comment as done.

XLenI32VT has i32 on RV32.

jrtc27 added inline comments.Mar 23 2021, 6:57 AM
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 added inline comments.Mar 23 2021, 1:13 PM
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 updated this revision to Diff 335415.Apr 5 2021, 11:54 PM

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 marked 3 inline comments as done.Apr 5 2021, 11:55 PM
Jim marked an inline comment as done.Apr 6 2021, 12:05 AM
Jim added inline comments.
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 added inline comments.Apr 6 2021, 9:50 AM
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 added inline comments.Apr 6 2021, 9:55 AM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
65

Is this line longer than 80 columns?

Jim updated this revision to Diff 336759.Mon, Apr 12, 12:18 AM
Jim marked an inline comment as done.

Use regular GPR class instead GPRP

Herald added a project: Restricted Project. · View Herald TranscriptMon, Apr 12, 12:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Jim updated this revision to Diff 338683.Mon, Apr 19, 7:17 PM

Rebase