This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add MC support of RISCV zcmt Extension
ClosedPublic

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 ↗(On Diff #463097)

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

llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
1

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

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 ↗(On Diff #463097)

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 ↗(On Diff #463097)

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

llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
1

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
367

This needs an assembler test

jrtc27 added inline comments.Sep 30 2022, 10:17 AM
llvm/lib/Target/RISCV/RISCV.td
366 ↗(On Diff #463097)

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

367 ↗(On Diff #463097)

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

Blank line

20

This should line up?

28

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

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

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 ↗(On Diff #463097)

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.

VincentWu updated this revision to Diff 497293.Feb 14 2023, 6:09 AM

update to v1.0

craig.topper added inline comments.Feb 14 2023, 11:15 PM
llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
131

Indent 1 more space

VincentWu updated this revision to Diff 510664.Apr 3 2023, 7:22 PM
VincentWu marked an inline comment as done.

rebase

VincentWu updated this revision to Diff 510667.Apr 3 2023, 7:36 PM

address comments

kito-cheng added inline comments.Apr 18 2023, 8:11 PM
lld/ELF/InputSection.h
404 ↗(On Diff #510667)

Why we need this change?

craig.topper added inline comments.Apr 18 2023, 9:59 PM
llvm/lib/Support/RISCVISAInfo.cpp
917

Extension names should be in single quotes.

Should we use 'zcmt' and 'c' extensions are incompatible to match line 863?

VincentWu updated this revision to Diff 516043.Apr 22 2023, 1:29 AM
VincentWu marked 2 inline comments as done.

address comment

VincentWu added inline comments.Apr 22 2023, 3:17 AM
lld/ELF/InputSection.h
404 ↗(On Diff #510667)

because zcmt have add a new elf section .riscv.jvt, that extent the size of class InputSection. So we have change it.

but I think it will be better if I move this change to https://reviews.llvm.org/D134600

kito-cheng added inline comments.Apr 24 2023, 2:43 AM
llvm/lib/Support/RISCVISAInfo.cpp
917

My understanding is Zcmt still compatible with C if no D/Zcd is present.

rv32ic_zcmt in theory is valid combination according to spec[1].

[1] https://github.com/riscv/riscv-code-size-reduction/blob/main/Zc-specification/Zc.adoc#19-zcmt

This extension reuses some encodings from c.fsdsp. Therefore it is incompatible with Zcd, which is included when C and D extensions are both present.

llvm/lib/Target/RISCV/RISCVFeatures.td
364–370

I would suggest do not check incompatible stuffs here, let RISCVISAInfo handle that, we already did that for others like Zfinx and F.

VincentWu updated this revision to Diff 516718.Apr 25 2023, 2:50 AM
VincentWu marked 2 inline comments as done.

rebase & address comments

VincentWu updated this revision to Diff 516722.Apr 25 2023, 2:56 AM

paraphrase grammar of error msg

This revision is now accepted and ready to land.Apr 25 2023, 9:04 PM
craig.topper requested changes to this revision.Apr 25 2023, 9:10 PM
craig.topper added inline comments.
llvm/lib/Support/RISCVISAInfo.cpp
855

This variable is unused.

915

It wouldn't be incompatible with Zca + D without Zcd would it?

This revision now requires changes to proceed.Apr 25 2023, 9:10 PM
VincentWu marked 2 inline comments as done.

address comment

craig.topper added inline comments.Apr 26 2023, 12:45 AM
llvm/lib/Support/RISCVISAInfo.cpp
917

The ‘or’ in the error message makes it unnecessarily complex for users. Tell them exactly what extensions they used that are in compatible.

craig.topper added inline comments.Apr 26 2023, 10:12 AM
llvm/lib/Support/RISCVISAInfo.cpp
918

'd' extensions - > 'd' extension

VincentWu updated this revision to Diff 518653.May 2 2023, 1:57 AM
VincentWu marked an inline comment as done.

address comments

This revision is now accepted and ready to land.May 2 2023, 10:50 AM
This revision was automatically updated to reflect the committed changes.
jrtc27 added inline comments.May 4 2023, 11:51 AM
llvm/test/MC/RISCV/rv32zcmt-invalid.s
9

This is wrong; the immediate must be in the range [32, 255]. This needs to be enforced in the assembler and the error needs to reflect that. We also need tests that check these cases (cm.jt with something in [32, 255] and cm.jalt with something in [0, 31]).