This is an archive of the discontinued LLVM Phabricator instance.

[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 RVP extension.
The encoding of RVP instructions is according to 0.9.11 version in https://github.com/riscv/riscv-p-spec/.

There are three sub-extensions in RVP.
zbpbo contains instructions overlap with the B extension.
zpn is a basic sub-extension. Most of instructions are belong to zpn.
zpsfoperand is the instruction will read or write 64-bit operands. On RV32, 64-bit operand is even/odd paired-register.
p = zbpbo + zpn + zpsfoperand

This patch has the tests for assemble/disassemble each instructions.

There are four test files for invalid assembly testing.
rvp-invalid.s is for invalid immediate operand in zpn extension for both RV32 and RV64.
rv32zpsfoperand-invalid.s shows that operand should be even register for even/odd paired-register operand on RV32.
rv64zpsfoperand-invalid.s are for invalid immediate operand in zpsfoperand extension on RV64.
rv64zpn-invalid.s are for invalid immediate operand for RV64 only instructions.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jim updated this revision to Diff 338683.Apr 19 2021, 7:17 PM

Rebase

Jim updated this revision to Diff 345340.May 13 2021, 8:12 PM

Rebase.
RVInstR4->RVInstR4Frm.

Jim updated this revision to Diff 349821.Jun 4 2021, 4:55 AM

Rebase

Jim updated this revision to Diff 351831.Jun 14 2021, 5:02 AM

Add tests for paired register operand for RV32.
Move testcases out of rvp directory.
Alias swap16 to pkbt16.

Jim updated this revision to Diff 352100.Jun 15 2021, 5:07 AM

Rebase

Jim edited the summary of this revision. (Show Details)Jun 15 2021, 5:59 AM
Jim added a comment.Jun 15 2021, 6:59 PM

Any comments? Thanks.

Jim updated this revision to Diff 353257.Jun 20 2021, 5:59 PM
Jim edited the summary of this revision. (Show Details)

Fix comment 0.9 to 0.93

Jim added a comment.Jun 21 2021, 3:17 AM

ping? Thanks.

craig.topper added inline comments.Jun 21 2021, 10:47 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
962

I think this can just be

return ((Reg - RISCV::X0) % 2) == 0 && Reg != RISCV::X0

Though the spec talks about what happens when X0_X1 is used as a source or dest, so should we really be forbidding X0?

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

Fold the 64-bit check into the if above with && to reduce nesting.

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

I think these opcodes are in numerical order, so please put this at the end to maintain that.

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

Are the positions correct? They don't match the 0.9.5-draft-20210617 pdf. Change log suggests it was modified in 0.5.3?

951

Why do these need EmitPriority = 2?

952

Can we give "def : SysReg<"vxsat", 0x009>;" a name in RISCVSystemOperands.td so we can reference it's encoding directly here like we do for these aliases

def : InstAlias<"rdinstret $rd", (CSRRS GPR:$rd, INSTRET.Encoding, X0)>;                                                                                                                                                                                                    
def : InstAlias<"rdcycle $rd",   (CSRRS GPR:$rd, CYCLE.Encoding, X0)>;                                                                                                                                                                                                      
def : InstAlias<"rdtime $rd",    (CSRRS GPR:$rd, TIME.Encoding, X0)>;
Jim updated this revision to Diff 353655.Jun 22 2021, 8:03 AM
  1. Address comments about coding style
  2. Append postfix valid to testcase file name
  3. Fix the changes of encoding in version 0.5.3
  4. Remove EmitPriority = 2 for rdov and clrov. It would emit no alias csrr rd, vxsat and csrci vxsat, 1 instead.
  5. Give a name to sys register vxsat for reference its encoding.
  6. Let X0 is a legal operand of even/odd paired-register. Add a dummy register ZERO to pair with X0. I am not sure that is a good implementation by adding a dummy register. X0 paired with X1 is not conformed with the spec.
Jim marked 6 inline comments as done.Jun 22 2021, 8:06 AM
Jim added a comment.Jun 26 2021, 2:21 AM

Any comments? Thanks.

Jim added a comment.Jul 4 2021, 7:29 PM

Ping? Thanks.

Jim updated this revision to Diff 356608.Jul 5 2021, 11:44 PM

Rebase

craig.topper added inline comments.Jul 7 2021, 11:57 AM
llvm/lib/Target/RISCV/RISCVInstrInfoP.td
600

It looks like the 0.9.3 spec lists this as an alias of MULSR64 and does not give it an encoding.

738

0.9.3 spec has this as an alias of KMAR64

836

I believe way this has been done on other extensions is to have (outs GPR:$rd_wb), (ins GPR:$rd, GPR:$rs1, uimmlog2xlenbytes:$shamt) and let Constraints = "$rd = $rd_wb"

And I don't think it should inherit from RVInstR since it doesn't have a real rs2.

This patch is nearly there! Just address the remaining review comments and it LGTM.

BTW, please mark all addressed inline comments as done. I think a few were missed, and it's helpful for a large patch like this.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
965–968

Nitpicking: perhaps this could use better terminology. You're asserting that Reg is a GPRPair, and then you convert Reg to a GPRPair, but the return value would fail an assert of isGPRPair...

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
69

What's the best way to address this?

llvm/test/MC/RISCV/rv32zpsfoperand-invalid.s
7–10

It would be nice to have a more specific error message, indicating the need for an even register operand. But not a deal-breaker, IMO.

Jim updated this revision to Diff 358159.Jul 12 2021, 10:38 PM

Address comments.

  1. Add alias for MULSR64 and KMAR64
  2. Fix instr format for INSB.
  3. Add new GPRPairOpOperand and implement parseGPRPairReg to report the need for an even register.
Jim marked 9 inline comments as done.Jul 12 2021, 10:40 PM
Jim added inline comments.Jul 12 2021, 10:45 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.td
69

sub_lo and sub_hi are only used for GPRPair register class to extract a register from pair registers on RV32.

Jim added inline comments.Jul 12 2021, 11:32 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.td
69

Do you mean that sub_lo and sub_hi only used on RV32? Does it need to rename or ..?

Jim updated this revision to Diff 360741.Jul 22 2021, 2:21 AM

Rebase

Jim added a comment.Jul 22 2021, 8:04 AM

ping? Thanks.

I think this looks good to me. @luismarques or @jrtc27 any further feedback?

jrtc27 added a comment.EditedJul 22 2021, 4:12 PM

I can't help but feel the assembly syntax for the register pair instructions should include both registers (perhaps in curly braces). The implicit use of the other register when reading the source is rather ugly, and particularly hard to remember when the RV64 versions *don't* do that. But that's a spec problem, not one with this patch, just something I'd like future spec revisions to give serious thought to...

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

The table is called RISCV32POnly but you're checking for Zpsfoperand (whatever that mouthful of an extension is). Which is it?

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

These aren't correct? RV64 doesn't have Zpsfoperand and RV32 doesn't have Zprvsfextra.

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

These lines look too long to me

177–178
232

Should probably have a TODO: to add scheduling information for these otherwise it'll get lost

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
69

Yes, these should have names that make it clear they're for each half of a 2*32-bit register pair. Otherwise it sounds like they're the 32-bit hi and lo halves of the 64-bit registers on RV64.

216

Ew, this is a gross quirk of the register pair instructions. ZERO is not a good name for it though, that's the ABI name for x0 so already taken and is pretty confusing. I assume LLVM doesn't like it if you create a register pair that is (X0, X0)?

226–228

IMO the register class should be GPR32Pair not GPRPair unless you also make it have a sensible interpretation for RV32 (which seems like a waste of time)

llvm/test/MC/RISCV/rv64zpn-invalid.s
7–25

This looks mostly the same as rv32zpn-invalid. The common tests should go in rv32zpn-invalid and have RV32 and RV64 RUN lines. Any truly RV32-only tests belong in something like rv32zpn-only-invalid. Any truly RV64-only tests can stay in rv64zpn-invalid.

Oh, technically none of the clang changes belong in this patch. Those are for the Clang driver and preprocessor, not the MC layer which is purely llvm.

gesysft removed a subscriber: gesysft.Jul 22 2021, 7:07 PM
Jim updated this revision to Diff 361095.Jul 22 2021, 11:14 PM
  1. Add TODO before instruction definitions that is lack scheduling information.
  2. Rename sub_lo and sub_hi to gpr_pair_lo and gpr_pair_hi.
  3. Rename ZERO register paired with X0 to REG_PAIR_WITH_X0 to avoid confusing.
  4. Rename GPRPair to GPR32Pair. It only is used for RV32.
  5. Move common invalid inst test to rvp-invalid.s. And add rv32zpn-only-invalid.s and rv64zpn-only-invalid.s.
Jim updated this revision to Diff 361096.Jul 22 2021, 11:17 PM

Add missing changes on previous update.

Jim edited the summary of this revision. (Show Details)Jul 22 2021, 11:31 PM

Oh, technically none of the clang changes belong in this patch. Those are for the Clang driver and preprocessor, not the MC layer which is purely llvm.

I move the clang changes to D95589 and D95590.

Jim updated this revision to Diff 361114.Jul 23 2021, 12:55 AM
Jim edited the summary of this revision. (Show Details)

Rename decode table name RISCV32POnly_ to RISCV32Zpsfoperand_.

Jim updated this revision to Diff 361116.Jul 23 2021, 1:08 AM
Jim marked 2 inline comments as done.

Apply git-clang-format

Jim marked 8 inline comments as done.Jul 23 2021, 1:12 AM
Jim added inline comments.
llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
442

Rename RISCV32POnly to RISCV32Zpsfoperand.
This is for the instruction with even/odd paired-register operands on RV32.

In RISCV32Zpsfoperand, two kinds of instruction defined for the same instruction in spec, one for RV32 with even/odd paired-register operands defined in RISCV32Zpsfoperand table , the other one for RV64 with normal GPR operands.

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

RV64 has Zpsfoperand extension that just has normal GPRs as operands (RV32 has even/odd paired-register operand).

RV32 doesn't have Zprvsfextra. All of instruction in Zprvsfextra are defined in Predicates = [HasStdExtZprvsfextra, IsRV64].

If P is enabled, it means Zpn+Zpsfoperand enabled on RV32, and Zpn+Zpsfoperand+Zprvsfextra enabled on RV64.

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
69

Rename it to gpr_pair_lo and gpr_pair_hi

216

Rename it to REG_PAIR_WITH_X0.

226–228

Rename it to GPR32Pair.

Jim marked 3 inline comments as done.Jul 23 2021, 1:15 AM
Jim added a comment.Jul 25 2021, 9:22 PM

@jrtc27 Any more feedback? Thanks.

Jim updated this revision to Diff 363366.Aug 1 2021, 8:25 PM

Rebase

jrtc27 added inline comments.Aug 1 2021, 8:28 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.td
69

The names still do not reflect the fact that they are only for RV32; currently it *looks* like on RV64 it'd give you the 64-bit GPR

Jim updated this revision to Diff 363441.Aug 2 2021, 4:38 AM
Jim added a comment.Aug 2 2021, 4:38 AM

Rename gpr_pair_lo and gpr_pair_hi to gpr32_pair_lo and gpr32_pair_hi for indicating only for RV32.

Jim marked an inline comment as done.Aug 2 2021, 4:41 AM
Jim added a comment.Aug 3 2021, 5:04 AM

Kindly ping? Thanks.

Jim updated this revision to Diff 365357.Aug 9 2021, 10:41 PM

Rebase

Jim added a comment.Aug 10 2021, 7:11 PM

Hi @jrtc27, any further feedback? Thanks.

jrtc27 added inline comments.Aug 10 2021, 7:31 PM
llvm/lib/Target/RISCV/RISCV.td
186–188

My concern is that the internal inflexibility is going to leak out of LLVM, such as into the .riscv.attributes section, and thus produce broken binaries because they claim they need Zprvsfextra on RV32, which is an invalid combination.

And if Zpsfoperand exists for RV64 then it shouldn't be called Paired-register operand 'P' Instructions, because the operands are only paired on RV32.

Jim updated this revision to Diff 366814.Aug 16 2021, 11:41 PM

Fix the description for Zpsfoperand to 64-bit operand 'P' Instructions

Jim marked an inline comment as done.Aug 16 2021, 11:55 PM
Jim added inline comments.
llvm/lib/Target/RISCV/RISCV.td
186–188

I upload a new patch https://reviews.llvm.org/D108189 to support P extension 's arch string.
If rv32izprvsfextra (Zprvsfextra on RV32) accepted from *.s arch attribute, it emits error message to forbid this invalid combination.
Before emitting _zprvsfextra string, it would check RV64 is enabled.

If llc is given -mattr=+experimental-zprvsfextra on RV32, it would be ignored.

Jim updated this revision to Diff 369385.Aug 30 2021, 12:09 AM
Jim marked an inline comment as done.

Rebase

Jim added a comment.Sep 2 2021, 11:12 PM

Hi, @jrtc27

About the concern the invalid combination RV32 + zprvsfextra on riscv attribute,
I upload a patch D108189 based on @kito-cheng 's patch D105168 to forbid emitting rv32+zprvsfextra arch string.
And It also deny to accept rv32izprvsfextra https://reviews.llvm.org/differential/changeset/?ref=2787775.

Patch D108189 have not emitted and accepted invalid arch string combined from rv32 and zprvsfextra.

Thanks for feedback.

Jim added a comment.Sep 7 2021, 11:26 PM

Ping? Thanks.

Jim updated this revision to Diff 372052.EditedSep 10 2021, 8:14 PM

Update to the newest P spec 0.96
https://github.com/riscv/riscv-p-spec/blob/master/P-ext-proposal.adoc#revision-history

  1. Merger Zprvsfextra (sub-extension for RV64 only) into Zpn So, RV32 and RV64 have the same sub-extension combination policy. P = Zpn + Zpsfoperand @jrtc27's concern is not existed now.
  2. Removed CLO* instructions.
  3. Fix incorrect testcases.
Jim edited the summary of this revision. (Show Details)Sep 10 2021, 8:20 PM
Jim edited the summary of this revision. (Show Details)
Jim edited the summary of this revision. (Show Details)
Jim updated this revision to Diff 372053.Sep 10 2021, 8:27 PM

Remove HasStdExtZprvsfextra and hasStdExtZprvsfextra() from RISCVSubtarget.h

Jim updated this revision to Diff 372054.Sep 10 2021, 8:34 PM

Fix incorrect -mattr for testcases.

Jim updated this revision to Diff 372057.Sep 10 2021, 10:54 PM

Move common invalid mc tests for both RV32 and RV64 to rvp-invalid.s

Jim marked 3 inline comments as done.Sep 10 2021, 10:57 PM
Jim edited the summary of this revision. (Show Details)Sep 10 2021, 11:01 PM
Jim added a comment.Sep 15 2021, 7:00 PM

Any feedback? I think this patch is good enough to be accepted.

Jim added a comment.Sep 21 2021, 7:26 PM
In D95588#3003090, @Jim wrote:

Any feedback? I think this patch is good enough to be accepted.

Ping? Thanks.

jrtc27 added inline comments.Sep 21 2021, 8:04 PM
llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
442

Is there a reason why this needs to be limited to Zpsfoperand? We don't have separate namespaces for every extension, we only add them when there are conflicts (with the "more common" extension being in the default namespace). I'd expect RISCV32Only_ with just a Feature64Bit check, like we do for some RV32C instructions, to be sufficient.

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

Isn't it 0.9.6 (or 0.9.7 now)? Not that that's a legal extension version number...

148

FOO32 vs FOO64 intuitively implies something about the width of operands to me, not what ISA they're for. For B the instruction definitions use _RV32 and _RV64 suffices.

Especially since the instructions are already called things like ADD64, this will generate nonsense ADD6432 and ADD6464 names...

170

This, RVPSMAL64Pair and RVPALU64Pair are all just RVPBinary with different numbers of the operands changed to GPR32PairOp, I think this would be better with default (to GPR) template arguments added to RVPBinary for rd/rs1/rs2 rather than having three almost-identical copies of the same thing under varying names

182

Similarly just make RVPTernary take template arguments for the types (in this case only one argument, for rd/rs3, needed)

910

Having the same instruction in two different extensions under two different names is insane. Currently this implementation lets you use the "wrong" name for kmar64 with Zpn. But I would prefer the spec weren't crazy.

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
219

Won't the order affect register allocation? Technically not an issue for this MC patch but the correct thing should be committed so there's no churn when adding codegen.

That or don't reuse this in the register class below and instead manually list the pairs in the class.

228

Why is this untyped?

Jim updated this revision to Diff 374748.Sep 24 2021, 1:13 AM
  1. Rename DecoderNamespace RISCV32Zpsoperand_ -> RISCV32Only_
  2. 0.96 -> 0.9.6
  3. Use suffices _32 and _64.
  4. Reuse RVPBinary and RVPTernary for GPR32PairOp operand
  5. Explict list register pair with allocation order
Jim updated this revision to Diff 374759.Sep 24 2021, 1:46 AM

Rename suffices _32 to _RV32 and _64 to _RV64.

Jim marked 7 inline comments as done.Sep 24 2021, 2:05 AM
Jim added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoP.td
910

I am still working on fixing it (from spec or ...) . But it is spec issue.

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
228

GPR32Pair has untyped type.
In code generation, It captures some operation with i64 type supported by Zpsoperand to untyped during legalization.
In my mind, untyped is used to be represented special type don't need any legalization.
I refer to ARMRegisterInfo.td that defines GPRPair with untyped.

jrtc27 added inline comments.Sep 24 2021, 8:43 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.td
219

You didn't need to manually list them all here...

228

So, presumably it can't be i64 because that would then make i64 a legal type? Maybe that's fine, maybe it's not, but alternatively it could be typed as v2i32 here (can include v4i16 and v8i8 too) and cast if/when needed. Craig maybe you have thoughts on this? But untyped always feels like laziness to me that results in TableGen being unable to help you when it comes to type checking, with rare exceptions.

Jim updated this revision to Diff 407771.Feb 10 2022, 9:57 PM
Jim marked an inline comment as done.

Rebase

Jim marked an inline comment as done.Feb 10 2022, 9:59 PM
Jim updated this revision to Diff 410235.Feb 21 2022, 12:07 AM

Remove inst alias smbb32 (alias to mulsr64) and kmar64 (alias to kmar64).
Having the same instruction in two different extensions under two different name is gone.

P-extension is not yet frozen. I am still cooperating with P-extension maintainer on this issue.

Jim marked 2 inline comments as done.Feb 21 2022, 12:56 AM
Jim added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoP.td
910

Remove alias inst first. I will discuss with P-extension maintainer on this issue.
Could this issue be fixed by a future revision?

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
228

All of operation node having untyped operands are customized SDNode lowered in RISCVISelLowering.cpp. And matched to a specific pattern.
untyped couldn't be as v2i32, v4i16 or v8i8 in the same time. The operand type has been checked in custom lowering.

The lowering for operation with specific operand types (v2i32, v4i16, v8i8 or i64 in this case) would be set cutsom and lowered to customized SDNode (This SDNode has untyped operands).
Different operand types with the same operation are lowered to different customized SDNode.

Jim marked an inline comment as done.Feb 21 2022, 1:01 AM
Jim added inline comments.
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-int.ll
4360 ↗(On Diff #410235)

The change for this testcase is that GPRJALR regclass is subsumed by GPR32Pair_with_gpr32_pair_lo_in_GPRNoX0X2 regclass.
And register pressure calculation is different.

Jim updated this revision to Diff 433613.Jun 1 2022, 6:35 PM

Rebase

Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 6:35 PM
sinan added a subscriber: sinan.Jun 5 2022, 11:20 PM

Hello @Jim! We are developing the P extension, are you still maintaining this patch? Or would you mind co-development together?

sunshaoce updated this revision to Diff 441927.Jul 3 2022, 1:41 AM

Support for P extension version 0.9.11

sunshaoce edited the summary of this revision. (Show Details)Jul 3 2022, 1:44 AM
sunshaoce updated this revision to Diff 441955.Jul 3 2022, 8:18 AM

Fix early-clobber-tied-def-subreg-liveness.mir

Jim added a comment.Jul 11 2022, 1:45 AM

Hello @Jim! We are developing the P extension, are you still maintaining this patch? Or would you mind co-development together?

Yes, I am still on developing the P extension. But it seems spec has the issue https://github.com/riscv/riscv-p-spec/issues/137 need to clarify.
And some instruction (CLZ, FSR and FSRI) in Zbpbo only be aliased to B extension on RV32. It is complicated if P extension are enabled on RV64.

In D95588#3641900, @Jim wrote:

Hello @Jim! We are developing the P extension, are you still maintaining this patch? Or would you mind co-development together?

Yes, I am still on developing the P extension. But it seems spec has the issue https://github.com/riscv/riscv-p-spec/issues/137 need to clarify.
And some instruction (CLZ, FSR and FSRI) in Zbpbo only be aliased to B extension on RV32. It is complicated if P extension are enabled on RV64.

Yes, this problem does exist. Thanks for your reply, looking forward to more cooperation!