This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add support for XCVmac extension in CV32E40P
ClosedPublic

Authored by realqhc on Jun 13 2023, 8:46 AM.

Details

Summary

Implement XCVmac intrinsics for CV32E40P according to the specification.

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

The patch-set aims at upstreaming the extensions on MC. The following will be on CodeGen, and the final patch-set will be on builtins if possible. The implemented version is on [0].

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

Spec: https://github.com/openhwgroup/cv32e40p/blob/62bec66b36182215e18c9cf10f723567e23878e9/docs/source/instruction_set_extensions.rst

[0] https://github.com/openhwgroup/corev-llvm-project

Diff Detail

Event Timeline

realqhc created this revision.Jun 13 2023, 8:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2023, 8:46 AM
realqhc requested review of this revision.Jun 13 2023, 8:46 AM
craig.topper added inline comments.Jun 13 2023, 1:57 PM
llvm/lib/Target/RISCV/RISCVInstrInfoXCvmac.td
1 ↗(On Diff #530924)

This line is 1 character longer than 80 characters

126 ↗(On Diff #530924)

Please fix the missing newline

llvm/test/MC/RISCV/corev/mac/invalid.s
5 ↗(On Diff #530924)

Please add {{$}} to the end of these lines so it checks to the end of the line.

llvm/test/MC/RISCV/corev/mac/mac.s
1 ↗(On Diff #530924)

Can we merge the per instruction tests into a single file? I think that's what's done for most extensions.

realqhc updated this revision to Diff 531074.Jun 13 2023, 2:48 PM

address reviewer's concerns, fixed format on RISCVInstrInfoXCvmac.td, reorganized the tests, and added checks for newline in error messages.

realqhc marked 2 inline comments as done.Jun 13 2023, 2:50 PM

Is this extension RV32 only? I notice the shift amounts are only uimm5. Should we restrict with Predicates = [IsRV32]?

craig.topper added inline comments.Jun 13 2023, 6:00 PM
llvm/lib/Target/RISCV/RISCVInstrInfoXCvmac.td
119 ↗(On Diff #531074)

Why Emit = 0 here?

realqhc added inline comments.Jun 13 2023, 11:20 PM
llvm/lib/Target/RISCV/RISCVInstrInfoXCvmac.td
119 ↗(On Diff #531074)

This is the current design for Core-V's GAS implementation. Also, setting it to non-zero may cause issue with cv.mulsn ..., ..., 0 but that may not exist apart from some test cases.

jrtc27 added inline comments.Jun 13 2023, 11:24 PM
llvm/lib/Target/RISCV/RISCVInstrInfoXCvmac.td
119 ↗(On Diff #531074)

Why are implementation details in gas at all relevant? This is LLVM not GNU binutils.

realqhc updated this revision to Diff 531224.Jun 14 2023, 1:15 AM

restrict the instruction to rv32 only, edit the documentations

realqhc updated this revision to Diff 531229.Jun 14 2023, 1:35 AM

address reviewers' concerns, restrict the instructions to RV32, remove unnecessary emit=0 and add -riscv-no-aliases for tests instead.

realqhc marked 2 inline comments as done.Jun 14 2023, 1:35 AM

I would say that this looks good to me, I only have nitpicks rather than any real issues.

I wonder whether we should write the human-readable version (and thus filenames for tests) of the extension as XCVmac rather than XCvmac. The argument would be that currently I read this almost as an extension called vmac for a vendor with the initial C. Though given we are writing XSfvcp maybe keeping it as it is would match that better. If no other reviewers have a preference I would say it's up to you. Either way the predicate text "'Xcvmac' (Multiply-Accumulate)" should be changed to match whatever you decide.

The final thing that also doesn't matter too much is that maybe the test filename for valid MC output should end in -valid.s.

realqhc updated this revision to Diff 531246.Jun 14 2023, 3:16 AM

implement naming change XCvmac->XCVmac, make error message consistent with naming.

realqhc retitled this revision from [RISCV] Add support for Xcvmac extension in CV32E40P to [RISCV] Add support for XCVmac extension in CV32E40P.Jun 14 2023, 3:17 AM
realqhc edited the summary of this revision. (Show Details)
realqhc updated this revision to Diff 531247.Jun 14 2023, 3:23 AM

make disassembler decoder table description more consistent.

realqhc updated this revision to Diff 531371.Jun 14 2023, 9:06 AM

re-name the file to RISCVInstrInfoXCV so that future Core-V extensions can be added in the same file.

realqhc updated this revision to Diff 531374.Jun 14 2023, 9:10 AM

fix header

realqhc edited the summary of this revision. (Show Details)Jun 16 2023, 12:57 AM
realqhc added a subscriber: xmj.
realqhc edited the summary of this revision. (Show Details)Jun 16 2023, 1:07 AM
craig.topper added inline comments.Jun 16 2023, 2:26 PM
llvm/test/MC/RISCV/corev/XCVmac-valid.s
3

Can we check disassembler too?

realqhc updated this revision to Diff 532339.Jun 16 2023, 5:34 PM

add test for objdump

realqhc marked an inline comment as done.Jun 16 2023, 5:35 PM
This revision is now accepted and ready to land.Jun 17 2023, 6:48 PM