Page MenuHomePhabricator

[RISCV] Add MC support of RISCV zcmt Extension
Needs ReviewPublic

Authored by VincentWu on Sep 14 2022, 7:39 AM.

Details

Summary

This patch add the instructions of zcmt extension.
spac is here
Which includes two instructions (cm.jt&cm.jalt) and a CSR Reg JVT

co-author: @Scott Egerton

Diff Detail

Event Timeline

VincentWu created this revision.Sep 14 2022, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2022, 7:39 AM
VincentWu requested review of this revision.Sep 14 2022, 7:40 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 14 2022, 7:40 AM
VincentWu updated this revision to Diff 460266.Sep 14 2022, 4:57 PM

fix test case

craig.topper added inline comments.Sep 26 2022, 10:18 AM
llvm/test/MC/RISCV/rv32zcmt-Invalid.s
5 ↗(On Diff #460266)

Why no invalid test for cm.jalt?

llvm/test/MC/RISCV/rv32zcmt-valid.s
9

What about CHECK-NO-EXT like other tests have?

llvm/test/MC/RISCV/rv64zcmt-Invalid.s
1 ↗(On Diff #460266)

Why is Invalid capitalized in this test name. It isn't capitalized in other MC tests is it?

1 ↗(On Diff #460266)

Merge this with the rv32 test.

llvm/test/MC/RISCV/rv64zcmt-valid.s
1 ↗(On Diff #460266)

Merge this with rv32zcmt-valid.s

VincentWu marked 5 inline comments as done.

update & address comments

reames added a subscriber: reames.Sep 28 2022, 10:30 AM

Please add to the review description a link to the appropriate specification. Please update docs/RISCVUsage.rst to add Zcmt, and link to the specification. It's impossible to review e.g. encoding without knowing what you're implementing.

llvm/lib/Target/RISCV/RISCV.td
367

Wait, Zcmt can't be enabled if C is? That seems odd...

llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
1 ↗(On Diff #463097)

We have Zca in RISCVInstrInfoC.td. Not sure it makes sense to have Zcmt in one place, and Zca in another.

Also, did Zca change between 0.7 and 0.7.5? If so, any plans to update that extension?

10 ↗(On Diff #463097)

It's not just one extension. It is several sub-extensions.

Please reword as "from the Zc* family of code size reduction extensions, version ...

craig.topper added inline comments.Sep 30 2022, 10:06 AM
llvm/lib/Target/RISCV/RISCV.td
367

I think Zcmt reuses encodings from the FP part of C. That's why C was split into Zca, Zcf, and Zcd.

craig.topper added inline comments.Sep 30 2022, 10:11 AM
llvm/lib/Target/RISCV/RISCV.td
367

This restriction needs to be enforced in RISCVISAInfo.cpp so that it generates a proper error message.

llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
1 ↗(On Diff #463097)

I think all Zc* should be in RISCVInstrInfoC.td. Zca/Zcf/Zcd already have to be in that file. Might as well put the rest there.

We should mention the Zc* extensions in the header of that file.

llvm/lib/Target/RISCV/RISCVSystemOperands.td
386

This needs an assembler test

jrtc27 added inline comments.Sep 30 2022, 10:17 AM
llvm/lib/Target/RISCV/RISCV.td
366

This is an odd implication, Zcmt works just fine without Zca?

367

Shouldn't the conflict be enforced at a higher level? Otherwise you're silently making +c,+zcmt resolve to +c here.

llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
19 ↗(On Diff #463097)

Blank line

20 ↗(On Diff #463097)

This should line up?

28 ↗(On Diff #463097)

I don't think we want this? cm.jt sym isn't a meaningful thing.

llvm/lib/Target/RISCV/RISCVSystemOperands.td
383

Capitalise (and maybe Jump Vector Table, assuming that's what jvt stands for? these headings come from the priv spec but that's woefully outdated for all the new asciidoc extensions...)

VincentWu updated this revision to Diff 469947.Oct 22 2022, 5:52 PM
VincentWu marked 12 inline comments as done.
VincentWu edited the summary of this revision. (Show Details)
  • update the document
  • Throw an error when +c and +zcmt are both specified
  • test CSR
llvm/lib/Target/RISCV/RISCV.td
366

the spec says:

The Zcmt extension depends on the Zca and Zicsr extensions.

I think maybe zcmt needs to be aligned using c.nop? That might be one of the reasons why zcmt required zca.