Page MenuHomePhabricator

[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

Unit TestsFailed

TimeTest
60 msx64 debian > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm

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.

180
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
179

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
253

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.

301

No tests were added for this one.

302

No tests were added for this one.

308

No tests were added for this one.

311

No tests were added for this one.

359

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
322

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

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

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

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.