Details

Reviewers

craig.topper
jrtc27
luismarques
HsiangKai
kito-cheng
asb
monkchiang

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

Repository: rG LLVM Github Monorepo

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

Harbormaster completed remote builds in B117424: Diff 363441.Aug 2 2021, 5:22 AM

Kindly ping? Thanks.

junparser added a subscriber: junparser.Aug 6 2021, 2:21 AM

Rebase

Harbormaster completed remote builds in B118809: Diff 365357.Aug 9 2021, 11:08 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.

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.

Harbormaster completed remote builds in B119840: Diff 366814.Aug 17 2021, 12:50 AM

Rebase

Harbormaster completed remote builds in B121704: Diff 369385.Aug 30 2021, 12:41 AM

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.

Ping? Thanks.

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

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.
Removed CLO* instructions.
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)

Remove HasStdExtZprvsfextra and hasStdExtZprvsfextra() from RISCVSubtarget.h

Fix incorrect -mattr for testcases.

Harbormaster completed remote builds in B123543: Diff 372054.Sep 10 2021, 9:11 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

Harbormaster completed remote builds in B123546: Diff 372057.Sep 10 2021, 11:29 PM

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

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?

jrtc27 mentioned this in D93298: [RISCV] add the MC layer support of Zfinx extension.Sep 21 2021, 8:15 PM

Rename DecoderNamespace RISCV32Zpsoperand_ -> RISCV32Only_
0.96 -> 0.9.6
Use suffices _32 and _64.
Reuse RVPBinary and RVPTernary for GPR32PairOp operand
Explict list register pair with allocation order

Harbormaster completed remote builds in B125493: Diff 374748.Sep 24 2021, 1:13 AM

Rename suffices _32 to _RV32 and _64 to _RV64.

Harbormaster completed remote builds in B125501: Diff 374759.Sep 24 2021, 1:46 AM

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.

Rebase

Herald added subscribers: • pcwang-thead, eopXD, VincentWu and 2 others. · View Herald TranscriptFeb 10 2022, 9:57 PM

Jim marked an inline comment as done.Feb 10 2022, 9:59 PM

Harbormaster completed remote builds in B148913: Diff 407771.Feb 10 2022, 10:20 PM

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.

Harbormaster completed remote builds in B150647: Diff 410235.Feb 21 2022, 12:07 AM

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.

Rebase

Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 6:35 PM

Herald added subscribers: sunshaoce, StephenFan, arichardson. · View Herald Transcript

Harbormaster completed remote builds in B167417: Diff 433613.Jun 1 2022, 7:17 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?

Support for P extension version 0.9.11

sunshaoce edited parent revisions, added: D108189: [RISCV] Support experimental 'P' extension 0.9.11; removed: D100288: [RISCV] Add vector types to GPR for P extension and explict type to existing codegen patterns.Jul 3 2022, 1:42 AM

sunshaoce edited the summary of this revision. (Show Details)Jul 3 2022, 1:44 AM

Harbormaster completed remote builds in B173431: Diff 441927.Jul 3 2022, 2:19 AM

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

Harbormaster completed remote builds in B173455: Diff 441955.Jul 3 2022, 9:18 AM

Rebase

Harbormaster completed remote builds in B174602: Diff 443553.Jul 10 2022, 10:13 PM

In D95588#3622052, @sunshaoce 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.

In D95588#3641900, @Jim wrote:

In D95588#3622052, @sunshaoce 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!

sunshaoce added inline comments.Oct 24 2022, 11:21 PM

llvm/lib/Target/RISCV/RISCVInstrInfoP.td
599	`0b011` should be `0b010`. https://github.com/riscv/riscv-p-spec/blob/master/P-ext-proposal.adoc#847-ukstsa32-simd-32-bit-unsigned-saturating-straight-subtraction--addition

pineapplehunter added a subscriber: pineapplehunter.May 12 2023, 10:24 AM

Herald added subscribers: jobnoorman, luke. · View Herald TranscriptMay 12 2023, 10:24 AM

asb mentioned this in D153974: [RISCV] Don't include X1 in the X0_PD register pair.Jun 28 2023, 7:04 AM

asb mentioned this in rG48a749e26e6f: [RISCV] Don't include X1 in the X0_PD register pair.Jul 20 2023, 5:49 AM

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 443553

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

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

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

llvm/lib/Target/RISCV/RISCV.td

llvm/lib/Target/RISCV/RISCVInstrFormats.td

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

llvm/lib/Target/RISCV/RISCVInstrInfo.td

llvm/lib/Target/RISCV/RISCVInstrInfoP.td

llvm/lib/Target/RISCV/RISCVRegisterInfo.td

llvm/lib/Target/RISCV/RISCVSubtarget.h

llvm/lib/Target/RISCV/RISCVSystemOperands.td

llvm/test/CodeGen/RISCV/attributes.ll

llvm/test/CodeGen/RISCV/early-clobber-tied-def-subreg-liveness.mir

llvm/test/MC/RISCV/rv32zpsfoperand-invalid.s

llvm/test/MC/RISCV/rv32zpsfoperand-valid.s

llvm/test/MC/RISCV/rv64zpn-invalid.s

llvm/test/MC/RISCV/rv64zpn-valid.s

llvm/test/MC/RISCV/rv64zpsfoperand-aliases-valid.s

llvm/test/MC/RISCV/rv64zpsfoperand-invalid.s

llvm/test/MC/RISCV/rv64zpsfoperand-valid.s

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

llvm/test/MC/RISCV/rvp-non-simd-valid.s

llvm/test/MC/RISCV/rvp-partial-simd-valid.s

llvm/test/MC/RISCV/rvp-simd-alu-valid.s

llvm/test/MC/RISCV/rvp-simd-cmp-valid.s

llvm/test/MC/RISCV/rvp-simd-misc-valid.s

llvm/test/MC/RISCV/rvp-simd-mul-valid.s

llvm/test/MC/RISCV/rvp-simd-shift-valid.s

llvm/test/MC/RISCV/rvp-simd-unpacking-valid.s

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[RISCV] Implement the MC layer support of P extensionNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 443553

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

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

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

llvm/lib/Target/RISCV/RISCV.td

llvm/lib/Target/RISCV/RISCVInstrFormats.td

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

llvm/lib/Target/RISCV/RISCVInstrInfo.td

llvm/lib/Target/RISCV/RISCVInstrInfoP.td

llvm/lib/Target/RISCV/RISCVRegisterInfo.td

llvm/lib/Target/RISCV/RISCVSubtarget.h

llvm/lib/Target/RISCV/RISCVSystemOperands.td

llvm/test/CodeGen/RISCV/attributes.ll

llvm/test/CodeGen/RISCV/early-clobber-tied-def-subreg-liveness.mir

llvm/test/MC/RISCV/rv32zpsfoperand-invalid.s

llvm/test/MC/RISCV/rv32zpsfoperand-valid.s

llvm/test/MC/RISCV/rv64zpn-invalid.s

llvm/test/MC/RISCV/rv64zpn-valid.s

llvm/test/MC/RISCV/rv64zpsfoperand-aliases-valid.s

llvm/test/MC/RISCV/rv64zpsfoperand-invalid.s

llvm/test/MC/RISCV/rv64zpsfoperand-valid.s

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

llvm/test/MC/RISCV/rvp-non-simd-valid.s

llvm/test/MC/RISCV/rvp-partial-simd-valid.s

llvm/test/MC/RISCV/rvp-simd-alu-valid.s

llvm/test/MC/RISCV/rvp-simd-cmp-valid.s

llvm/test/MC/RISCV/rvp-simd-misc-valid.s

llvm/test/MC/RISCV/rvp-simd-mul-valid.s

llvm/test/MC/RISCV/rvp-simd-shift-valid.s

llvm/test/MC/RISCV/rvp-simd-unpacking-valid.s

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

Revision Contents
Changeset List