This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add support for XCValu extension in CV32E40P
ClosedPublic

Authored by realqhc on Jun 26 2023, 1:45 AM.

Details

Summary

Implement XCValu intrinsics for CV32E40P according to the specification.

This is a commit of the patch-set to upstream the 7 vendor specific extensions of CV32E40P.

Contributors: @CharKeaney, Nandni Jamnadas, Serkan Muhcu, @jeremybennett, @lewis-revill, @liaolucy, @simoncook, @xmj

Diff Detail

Event Timeline

realqhc created this revision.Jun 26 2023, 1:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 1:45 AM
realqhc requested review of this revision.Jun 26 2023, 1:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 1:45 AM
realqhc edited the summary of this revision. (Show Details)Jul 3 2023, 1:01 AM

You might need to add extension in RISCVISAInfo.cpp for -march support?

llvm/docs/RISCVUsage.rst
278
realqhc added inline comments.Jul 4 2023, 1:11 AM
llvm/docs/RISCVUsage.rst
278

This might need to be updated as if you check https://github.com/openhwgroup/cv32e40p/releases, there has been release of v1.3.2 last week.

You might need to add extension in RISCVISAInfo.cpp for -march support?

We plan to add extension to RISCVISAInfo.cpp in the second batch of upstreaming patches along with the LLVM Intrinsics for the extension.

kito-cheng added inline comments.Jul 4 2023, 1:44 AM
llvm/docs/RISCVUsage.rst
278

I saw binutils from openhwgroup still using 1.0[1], and that patch is committed 5 days ago? I am not intend to block this patch, but I would like to figure out what's the right version for those extension.

[1] https://github.com/openhwgroup/corev-binutils-gdb/commit/9b120787924eed13455de3062523539dc39bc7f8

craig.topper added inline comments.Jul 4 2023, 2:09 AM
llvm/docs/RISCVUsage.rst
278

The cv.mac part was copied from another description and does not apply here.

realqhc added inline comments.Jul 4 2023, 2:12 AM
llvm/docs/RISCVUsage.rst
278

Should I change it to cv. like XCVbitmanip or do you think cv.alu is more appropriate here?

liaolucy added inline comments.Jul 4 2023, 2:21 AM
llvm/docs/RISCVUsage.rst
278

Thanks, about the version, we have a group meeting on Friday, we will confirm it internally and come back to synchronize the information

realqhc updated this revision to Diff 536999.Jul 4 2023, 2:26 AM

fix error in riscvusage.rst to reflect actual prefix of XCValu extension instructions.

realqhc marked an inline comment as done.Jul 4 2023, 2:36 AM
jeremybennett added inline comments.Jul 4 2023, 8:33 AM
llvm/docs/RISCVUsage.rst
278

The document referred to is the entire specification of the CV32E40P, of which its ISA extensions are just a small section. The main document is subject to numerous small changes, and bumps its minor version and patch version from time to time. However the ISA extensions from a software perspective have a single version, 1.0, which is the only version that will be supported by the tool chain.

I'm open to advice on how we should handle this. I note that XCVbitmanip and XCVmac have both been merged using the full document reference (1.3.1). It might be clearer to reference the software spec on ISA extension naming, which explicitly notes that versioning is *not* supported for CORE-V: https://github.com/openhwgroup/core-v-sw/blob/master/specifications/corev-isa-extension-naming.md

craig.topper added inline comments.Jul 4 2023, 1:35 PM
llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
184

Can we name all of these CVInst* instead of RVInst* to match some of the earlier classes like CVBitManipRII?

204

Can this inherit from RVInstR which will simplify a lot of the body.

234

Can this inherit from RVInstR with a let rs2 = 0b00000?

You might need to add extension in RISCVISAInfo.cpp for -march support?

We plan to add extension to RISCVISAInfo.cpp in the second batch of upstreaming patches along with the LLVM Intrinsics for the extension.

I think the assembler also needs to RISCVISAInfo.cpp changes to print the correct version for the ELF attributes to list what extensions are enabled in the binary.

craig.topper added inline comments.Jul 4 2023, 6:53 PM
llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
325

Please wrap this to 80 columns and in general try to keep lines close to 80 columns.

You might need to add extension in RISCVISAInfo.cpp for -march support?

We plan to add extension to RISCVISAInfo.cpp in the second batch of upstreaming patches along with the LLVM Intrinsics for the extension.

I think the assembler also needs to RISCVISAInfo.cpp changes to print the correct version for the ELF attributes to list what extensions are enabled in the binary.

Thanks for the suggesion, will get the versioning updated after discussion with core-v.

realqhc updated this revision to Diff 537200.Jul 4 2023, 7:35 PM

rename RVInst to CVInst, add core-v to instruction description, use RVInstR to simplify CVInstALU_rr and CVInstAlu_r

realqhc updated this revision to Diff 537632.Jul 6 2023, 2:16 AM

update tests

jeremybennett added inline comments.Jul 7 2023, 12:45 AM
llvm/docs/RISCVUsage.rst
278

Just to clarify. The CORE-V ISA extensions do not support versioning, which in a RISC-V context means the version is always 1.0.

realqhc updated this revision to Diff 538044.Jul 7 2023, 2:32 AM
realqhc marked 4 inline comments as done.

update ISA version accordingly

realqhc marked 5 inline comments as done.Jul 7 2023, 2:32 AM
craig.topper added inline comments.Jul 7 2023, 12:36 PM
llvm/lib/Target/RISCV/RISCVFeatures.td
772

Can you put CORE-V in this string too

llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
233

I would have expected cv.sle and cv.sleu for these mnemonics. The t feels out of place. In the base isa for slt/sltu it's part of "less than". Is the instruction name here "set less equal than" instead "set less than or equal"?

The vector spec uses "sle" and "sleu"

Is the spec frozen?

260

I'm surprised this instruction exists. Isn't this andi rs1, 255?

305

Wrap to 80 columns. by putting (ins GPR:$rd, GPR:$rs1, GPR:$rs2), on a new line.

realqhc updated this revision to Diff 538321.Jul 7 2023, 10:28 PM
realqhc marked an inline comment as done.

fix the problem of code being wrongly wrapped to 120 columns

realqhc updated this revision to Diff 538967.Jul 11 2023, 2:03 AM

fix previous errorneus update

realqhc updated this revision to Diff 542327.Jul 20 2023, 12:05 AM

applied general cleanup like in D155283, rebase.

realqhc marked 3 inline comments as done.Jul 20 2023, 12:09 AM
realqhc added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
233

The current naming is copied from PULP RI5CY and it is close to be frozen[1].
[1]: https://github.com/openhwgroup/cv32e40p/issues/833

260

I believe this is also a leftover from PULP RI5CY.

realqhc marked 2 inline comments as done.Jul 20 2023, 12:14 AM
realqhc updated this revision to Diff 543813.Jul 24 2023, 9:16 PM

remove Opcode to reflect changes

craig.topper accepted this revision.Jul 25 2023, 3:43 PM

LGTM

llvm/docs/RISCVUsage.rst
277

Keep this with the other XCV extensions

This revision is now accepted and ready to land.Jul 25 2023, 3:43 PM
realqhc updated this revision to Diff 544861.Jul 27 2023, 11:04 AM

Simplify tablegen for alu

realqhc updated this revision to Diff 544863.Jul 27 2023, 11:08 AM

move xcvalu in RISCVUsage.rst so that it is with other XCV extensions

realqhc marked an inline comment as done.Jul 27 2023, 11:08 AM