According the newest RISC-V Privileged Spec, updated CSRs.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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
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:
| |
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. | |
303 | No tests were added for this one. | |
304 | No tests were added for this one. | |
310 | No tests were added for this one. | |
313 | No tests were added for this one. | |
361 | It's a bit tedious, but a quick script should generate something appropriate. These CSRs have no tests. |
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 | ||
---|---|---|
324 | 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. |
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." |
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.