[RISCV] Add MC support of RISCV zcmt Extension

[VincentWu]

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

[VincentWu]

fix test case

[craig.topper]

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]

update & address comments

[reames]

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]

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]

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

386

This needs an assembler test

[jrtc27]

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

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]

• 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]

update to v1.0

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfoZc.td

131

Indent 1 more space

[VincentWu]

rebase

[VincentWu]

address comments

[kito-cheng]

lld/ELF/InputSection.h

404 ↗

(On Diff #510667)

Why we need this change?

[craig.topper]

llvm/lib/Support/RISCVISAInfo.cpp

915

Extension names should be in single quotes.

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

[VincentWu]

address comment

[VincentWu]

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]

llvm/lib/Support/RISCVISAInfo.cpp

915

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

354–360

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

[VincentWu]

rebase & address comments

[VincentWu]

paraphrase grammar of error msg

[kito-cheng]

LGTM

[craig.topper]

llvm/lib/Support/RISCVISAInfo.cpp

853

This variable is unused.

913

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

[VincentWu]

address comment

[craig.topper]

llvm/lib/Support/RISCVISAInfo.cpp

915

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]

llvm/lib/Support/RISCVISAInfo.cpp

916

'd' extensions - > 'd' extension

[VincentWu]

address comments

[craig.topper]

LGTM

[jrtc27]

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

10

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]).