riscv / riscv-isa-manual

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

Wavedrom encodings are inconsistently formatted #1430

Open christian-herber-nxp opened 4 months ago

christian-herber-nxp commented 4 months ago

I am failing to understand the logic behind the formatting of instruction encodings in wavedrom. For one, it is quite inconsistent between extensions (e.g. compare C to Zc). But even within extensions, or even a single instruction, I do not understand how the type is assigned (that translates to the background color). See e.g.: image

Two parts of one immediate have different colors. One operand red, one operand green.

Having clarity on how to format the instructions will greatly help authors of extensions to do it right from the start.

wmat commented 4 months ago

On Tue, May 28, 2024 at 4:38 AM Christian Herber @.***> wrote:

I am failing to understand the logic behind the formatting of instruction encodings in wavedrom.

While converting LaTeX to AsciiDoc, I attempted to reuse the types that had already existed in wavedrom that had been written prior to taking over the spec conversion.

For one, it is quite inconsistent between extensions (e.g. compare C to Zc).

Some newer specs that have been integrated may have incorrect wavedrom types (background colors). This is something that should be cleaned up on an ongoing basis.

But even within extensions, or even a single instruction, I do not understand how the type is assigned (that translates to the background color). See e.g.: image.png (view on web) https://github.com/riscv/riscv-isa-manual/assets/101172126/5492c960-8e8e-4517-80e2-b96282b44e11

Two parts of one immediate have different colors. One operand red, one operand green.

Having clarity on how to format the instructions will greatly help authors of extensions to do it right from the start.

I created a key (albeit incomplete) for my own use based on previously created wavedrom diagrams. I'm not sure where these types came from, however, I attempted to be as consistent as possible during the conversion process:

Name Type Colour Imm 3 light green rs1 4 teal rs2 4 teal width 8 grey opcode 8 grey rd 2 light red rm 8 fmt 8 funct5 8

— Reply to this email directly, view it on GitHub https://github.com/riscv/riscv-isa-manual/issues/1430, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAN6ZEGYLJB6OXTNK24AZDZEQ7BPAVCNFSM6AAAAABIMOFROOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGMZDANBWHE2TAMI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

ved-rivos commented 4 months ago

I feel the color code is arbitrary and perhaps not intentional. The version 2.2 manual had no such color scheme.

christian-herber-nxp commented 4 months ago

@wmat : thanks, i updated the colors in https://github.com/riscv/riscv-zilsd/pull/18. From that, I can for sure say that the types in C need to be revisited.

gfavor commented 4 months ago

Given that the "old" ISA manuals did not use color coding, should we drop all color coding in the new manuals? Or should we have Bill identify one standard color coding that we will gradually convert to?

ved-rivos commented 4 months ago

IMO: The colors are a bit distracting and don't add much. The look and feel of the "old" manual was better.

wmat commented 4 months ago

The addition of colored backgrounds to the wavedrom diagrams predated my joining the project. I always assumed it was done for a reason but perhaps not. Removing the colorization (type attributes) from the wavedrom would certainly make writing wavedrom that much easier.

On Tue, May 28, 2024 at 12:47 PM Ved Shanbhogue @.***> wrote:

IMO: The colors are a bit distracting and don't add much. The look and feel of the "old" manual was better.

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

gfavor commented 4 months ago

Let's see if @aswaterman concurs with dropping all color coding.

christian-herber-nxp commented 4 months ago

personally, I like the colors, if used meaningfully and consistently. Especially in less regular encodings like C/Zc, helps to get an overview.

aswaterman commented 4 months ago

I think the two valid resolutions are to drop the colors or to use the colors consistently. Given the latter option is much more work, and isn't exactly of high priority, I'd be fine with dropping them. We can always reverse course in the future if we have the bandwidth.

wmat commented 4 months ago

I assume the colors are simply decoration, correct?

Bill Traynor Documentation Build and Release Engineer RISC-V International

Join us in Munich, Germany at RISC-V Summit Europe https://riscv-europe.org/summit/2024/ from 24-28 June. Be a part of the new wave of European computing innovation!

On Tue, May 28, 2024 at 5:07 PM Andrew Waterman @.***> wrote:

I think the two valid resolutions are to drop the colors or to use the colors consistently. Given the latter option is much more work, and isn't exactly of high priority, I'd be fine with dropping them. We can always reverse course in the future if we have the bandwidth.

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

aswaterman commented 4 months ago

Right, they’re just there to improve the visual appearance and help readers see the boundaries between fields. There’s no semantic content.