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 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 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

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
354–356

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
354–356

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
459

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
198

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.

207

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
207

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
198

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

207

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
207

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!