riscv / riscv-isa-manual

RISC-V Instruction Set Manual
https://riscv.org/
Creative Commons Attribution 4.0 International
3.57k stars 617 forks source link

Enhance RISC-V Instruction Documentation #1590

Open AFOliveira opened 1 month ago

AFOliveira commented 1 month ago

Building on issues #1470 #1369 #1253 #1396 #1567 and aligning with the RISC-V Unified Database efforts, I've made some tweaks that I believe could potentially improve the description of all instructions in the manual. Currently, instruction description generally follows this structure:

Screenshot 2024-08-08 112122

IMO information is too condensed and this makes it a bit harder to understand, so I believe that the following example of the following standard is a much better alternative:

image

This approach clarifies instruction descriptions and can be efficiently generated using existing tools. By leveraging riscv-opcodes (particularly parse.py) and sail-riscv, I've automated the creation of instruction-per-page documentation. The process extracts encodings, mnemonics, and argument lists from riscv-opcodes, then obtains and post-processes SAIL code from sail-riscv for each instruction, maintaining readability and executability.

The main challenge with this approach lies in the SAIL code's structure. SAIL, written as conventional code, lacks certain conventions that would facilitate easier parsing and post-processing. While it's possible to work with the current SAIL repository, doing so would be time-consuming and less adaptable to future changes in riscv-sail/model. Implementing specific conventions could significantly streamline this process.

As an early stage work, this approach currently covers a subset of RISC-V instructions. The preliminary set of supported instructions can be found in this RISC-V instructions PDF.

The branch where this work can be checked is SAIL WiP, but please take it only as a Proof of Concept since it is not finished.

wmat commented 1 month ago

Interesting effort. In looking at the result, some of the encoding diagrams are messed up with text overlaying the boundaries of the bit they're in, for example the BEQ instruction. What would be really useful is if you could generate two .adoc files for inclusion as an indices in the Unprivileged and Privileged manuals.

AFOliveira commented 1 month ago

Thank you so much for your feedback! I'm aware of some existing bugs, including the one you've highlighted. This specific issue might be resolved as current encodings do:

image

Creating two-document support for the instructions index is definitely viable with this approach. I've made initial code changes, but I need to review their scalability across the entire ISA.

I can't upload the .asciidoc here so please check if these two small versions of the Unprivileged Instruction Index and Privileged Instruction Index correctly address your feedback.

wmat commented 1 month ago

Those seem to satisfy the split. How are you getting the SAIL code, btw? I don't see any include statments in the code, nor do I see a SAIL index.

AFOliveira commented 1 month ago

@wmat sorry for the delay. The current approach for obtaining SAIL code assumes the root folders for riscv-opcodes and sail-riscv repositories are the same. In the parse.py file, the make_asciidoc_sail function specifically references one directory of the SAIL repo.

We're referencing a specific file in this version due to variable naming in SAIL code. One of the main flaws of this approach is that there doesn't seem to be an appropriate variable naming convention that would allow easy correlation between both repositories.

While it's possible to correlate all files this way, doing so might make the code less flexible and require constant updates when new extensions are added.

I believe that minimal changes on both sail-riscv and riscv-opcodes repositories could hugely enhance this approach for documentation. However, since those are functional repos, I prefer to ask for support here to check if minimally changing those might be a possibility.

edolnx commented 3 weeks ago

Adding references to the SAIL repoistory for every instruction/CSR is something I am working towards as part of the Unified DB project. It's not going to be perfect, but it will make it easier to find and address those as the Golden Model project break apart more of their code base.

wmat commented 3 weeks ago

On Mon, Aug 26, 2024 at 1:11 PM Carl Perry @.***> wrote:

Adding references to the SAIL repoistory for every instruction/CSR is something I am working towards as part of the Unified DB project. It's not going to be perfect, but it will make it easier to find and address those as the Golden Model project break apart more of their code base.

This is exactly what the asciidoctor-sail extension is for. https://github.com/Alasdair/asciidoctor-sail

For the record, I'm trying to document the how to make it easier for extension writers to reference SAIL.

— Reply to this email directly, view it on GitHub https://github.com/riscv/riscv-isa-manual/issues/1590#issuecomment-2310677584, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAN6ZBSRSX57GLZ5MESVPLZTNOUTAVCNFSM6AAAAABMGNCIJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJQGY3TONJYGQ . You are receiving this because you were mentioned.Message ID: @.***>

edolnx commented 3 weeks ago

I'd also like to expand upon this layout as a basis for all instructions moving forward. Several of us are working on a goal to have these pages generated from the Unified DB, and there are cross-extension additions that I'm trying to address. Especially as we move forward with the model of One Single Repo for all the ISA Specification where all new extensions are created in a branch, I think it's critical that we reduce the scope of changes otherwise the merge process will be excruciating. To that point, there are extensions that modify the behavior of existing instructions/CSRs/extensions. Ideally we should be able to "top insert" or "bottom append" information to a Documentation Block to the affected instructions/CSRs/extensions from a new instruction/CSR/extension via the Unified DB. But this can only happen if we have a well defined set of Blocks for each page so that they can be targeted for "top insert" or "bottom append". What is listed in this design is a good starting point, but we should probably look a little wider and ensure we have all the necessary blocks well defined, even if they are blank, in every page.

edolnx commented 3 weeks ago

On Mon, Aug 26, 2024 at 1:11 PM Carl Perry @.> wrote: Adding references to the SAIL repoistory for every instruction/CSR is something I am working towards as part of the Unified DB project. It's not going to be perfect, but it will make it easier to find and address those as the Golden Model project break apart more of their code base. This is exactly what the asciidoctor-sail extension is for. https://github.com/Alasdair/asciidoctor-sail For the record, I'm trying to document the how to make it easier for extension writers to reference SAIL. — Reply to this email directly, view it on GitHub <#1590 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAN6ZBSRSX57GLZ5MESVPLZTNOUTAVCNFSM6AAAAABMGNCIJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJQGY3TONJYGQ . You are receiving this because you were mentioned.Message ID: @.>

I was unaware of the existence of this, thanks for the info. I look forward to reading more about it and how to integrate it.

AFOliveira commented 3 weeks ago

To that point, there are extensions that modify the behavior of existing instructions/CSRs/extensions. Ideally we should be able to "top insert" or "bottom append"

I believe this for sure have space there, as I referenced earlier this is just a base in which things can be added or removed. Talking with @dhower-qc we already got to the conclusion that if the description is good enough, there might not be a point in having an argument list, and therefore we removed it in the yaml version. I have been working in this yaml version which is on riscv-software-src/riscv-unified-db/pull/21 that generates the same thing that is here but in .yaml. I think we can definitely add that parameter both here and there. I think I will add that to the #1603 and reference the new comments here.