Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix formatting in "RISC-V base unprivileged register state" table #1784

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

radimkrcmar
Copy link
Contributor

I don't understand how those XLENs even got there.

The "RISC-V base unprivileged integer register state" table has stray
XLEN fields and an unnecessary and mis-formatted mid-table header.
Remove them all.

Signed-off-by: Radim Krčmář <[email protected]>
@wmat
Copy link
Collaborator

wmat commented Dec 30, 2024

The table was formatted to match the table from the 2019 spec, see screenshot:

Screenshot From 2024-12-30 08-47-12

I'll leave it to @aswaterman to determine which is correct.

@radimkrcmar
Copy link
Contributor Author

The table was formatted to match the table from the 2019 spec, see screenshot:

Having XLEN there makes sense, thanks. The current table is even worse than I thought as it doesn't put the header outside either.

I applied bytefield to make it look just like other register definitions, but that doesn't break on page boundary and would move the whole table the next page, leaving awkward white space:
screenshot-2024-12-30T16:49:26+01:00

Is this solution acceptable?

@wmat
Copy link
Collaborator

wmat commented Dec 30, 2024

So from a content perspective, is the original table from the 2019 spec incorrect?

@radimkrcmar
Copy link
Contributor Author

So from a content perspective, is the original table from the 2019 spec incorrect?

No, that one is good and I will add a separate table for the pc register if that is preferred. (Seemed unnecessary.)
The problem is that the current one looks different from 2019. Table header is inconsistent with other definitions and what happens around the pc register is just confusing:
screenshot-2024-12-31T08:58:17+01:00

@aswaterman
Copy link
Member

Right, the bug is that two of the XLENs are now cells within the table, whereas in the original they were text below the table.

@wmat I’m OK with any solution that doesn’t make it look like the XLENs are rows in the table. If that requires deleting these two XLENs (but of course retaining the XLEN-1 in the top-left corner), so be it. I’ll leave this to you.

@wmat
Copy link
Collaborator

wmat commented Dec 31, 2024

@radimkrcmar if you can do as Andrew suggests, that'd be great.

@radimkrcmar
Copy link
Contributor Author

@radimkrcmar if you can do as Andrew suggests, that'd be great.

The patch does that: remove two lines before the pc register an one line after it, leaving the inconsistent header on top.
I would be happier with the solution presented in #1784 (comment), so I'll respin this pull request with it if you don't mind the white space.
(Floating point registers need the same change, so extra PR will land either way.)

@wmat wmat merged commit 7c5adda into riscv:main Jan 2, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants