This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][llvm] Update CSRs
ClosedPublic

Authored by achieveartificialintelligence on Jan 5 2022, 2:22 AM.

Details

Summary

According the newest RISC-V Privileged Spec, updated CSRs.

Diff Detail

Event Timeline

achieveartificialintelligence requested review of this revision.Jan 5 2022, 2:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2022, 2:22 AM
jrtc27 added inline comments.Jan 5 2022, 3:52 AM
llvm/lib/Target/RISCV/RISCVSystemOperands.td
176

You've got rid of the S-mode N extension CSRs but not the U-mode ones. I don't know if we should be removing these or not; I do know of a soft-core that actually implements it.

182
achieveartificialintelligence marked 2 inline comments as done.

Address @jrtc27 's comments.

asb added a comment.Jan 6 2022, 5:10 AM

I haven't gone through everything here, but I just spotted this is missing scountovf from the recently ratified Count Overflow and Mode-Based Filtering Extension.

I haven't gone through everything here, but I just spotted this is missing scountovf from the recently ratified Count Overflow and Mode-Based Filtering Extension.

I couldn't find scountovf in https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20211229-fb7e236 . Should I add this CSR?

asb added a comment.Jan 6 2022, 5:32 AM

I haven't gone through everything here, but I just spotted this is missing scountovf from the recently ratified Count Overflow and Mode-Based Filtering Extension.

I couldn't find scountovf in https://github.com/riscv/riscv-isa-manual/releases/tag/draft-20211229-fb7e236 . Should I add this CSR?

Oh I see, some of the CSRs from recently ratified extensions have been merged into the privileged spec but not all. Probably easiest to review if we keep this patch to just those CSRs added in the privilege spec update, and have separate patches for CSRs in other ratified extensions that aren't yet merged in.

Oh I see, some of the CSRs from recently ratified extensions have been merged into the privileged spec but not all. Probably easiest to review if we keep this patch to just those CSRs added in the privilege spec update, and have separate patches for CSRs in other ratified extensions that aren't yet merged in.

OK, thank you! @asb

asb added a comment.Jan 11 2022, 4:56 AM

Thanks for the patch - I've left a few comments regarding missing tests and a formatting issue. Then I think this should be good to go.

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

Nit: the comment heading used here and elsewhere in this patch doesn't match the rest of the file. I'd either:

  1. Modify the existing headings prior to this patch
  2. Stick to the style used by existing headings
255

I'm wondering if this should be isRV32Only. By my understanding, if you're compiling code to operate under HSXLEN=32 then you're effectively targeting RV32.

304

No tests were added for this one.

305

No tests were added for this one.

311

No tests were added for this one.

314

No tests were added for this one.

362

It's a bit tedious, but a quick script should generate something appropriate. These CSRs have no tests.

achieveartificialintelligence marked 7 inline comments as done.

Address @asb 's comments

asb added a comment.EditedJan 12 2022, 1:24 AM

Thanks for the update - just two more minor issues I noticed, then I think this is good to go (for real this time!).

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

The isRV32Only property isn't tested in a *-csr-names-invalid.s file.

llvm/test/MC/RISCV/user-csr-names-invalid.s
1

As this file is called user-csr-names-invalid.s, the tests below should arguably move into separate files.

Though I'm not sure there's a huge amount of value in spreading it out across multiple files, so perhaps it would make sense to rename this file to rv32-only-csr-names.s or similar and adjust the comment at the top about this only covering user mode CSR names.

achieveartificialintelligence marked 2 inline comments as done.

Thanks for your detailed comments. @asb

asb accepted this revision.Jan 12 2022, 3:47 AM

Thanks, LGTM (one teeny tiny nit regarding a comment in a test).

llvm/test/MC/RISCV/rv32-only-csr-names.s
5

Change to "The following CSR register names are all RV32 only."

This revision is now accepted and ready to land.Jan 12 2022, 3:47 AM
This revision was automatically updated to reflect the committed changes.