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

Adding wavedrom images of the encoding #18

Conversation

christian-herber-nxp
Copy link
Collaborator

No description provided.

@christian-herber-nxp christian-herber-nxp linked an issue May 28, 2024 that may be closed by this pull request
Copy link
Collaborator

@tovine tovine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to have these added, do we also have a pipeline to build it and see a rendered version?

images/wavedrom/c-load_store_pair.adoc Outdated Show resolved Hide resolved
images/wavedrom/c-load_store_pair.adoc Outdated Show resolved Hide resolved
@christian-herber-nxp
Copy link
Collaborator Author

Good to have these added, do we also have a pipeline to build it and see a rendered version?

Nope. I do think this is needed however, and i had sent a mail to riscv-help earlier today

@christian-herber-nxp
Copy link
Collaborator Author

Good to have these added, do we also have a pipeline to build it and see a rendered version?

Nope. I do think this is needed however, and i had sent a mail to riscv-help earlier today

for now, best thing during review is to build locally or to pase into https://wavedrom.com/editor.html

Copy link
Collaborator

@tovine tovine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@wmat
Copy link
Contributor

wmat commented May 28, 2024

Note that if you are building locally, be sure to do git submodule update --remote --init to pull in the submodule code or the local build will fail to find the theme and fonts.

As for the wavedrom, if you add --verbose to the Makefile and build locally, it spits out these errors:

asciidoctor: DEBUG: images/wavedrom/load_store_pair.adoc: line 4: unknown style for literal block: wavedrom
asciidoctor: DEBUG: images/wavedrom/load_store_pair.adoc: line 15: unknown style for literal block: wavedrom
asciidoctor: DEBUG: images/wavedrom/c-load_store_pair.adoc: line 4: unknown style for literal block: wavedrom
asciidoctor: DEBUG: images/wavedrom/c-load_store_pair.adoc: line 15: unknown style for literal block: wavedrom
asciidoctor: DEBUG: images/wavedrom/c-load_store_pair.adoc: line 25: unknown style for literal block: wavedrom
asciidoctor: DEBUG: images/wavedrom/c-load_store_pair.adoc: line 37: unknown style for literal block: wavedrom

I'm working on figuring out why.

@wmat
Copy link
Contributor

wmat commented May 28, 2024

OK, all fixed up. I pushed to this PR. The latest GH Action build now includes the generated wavedrom diagrams. https://github.com/riscv/riscv-zilsd/actions/runs/9271407039

The Makefile was missing some build requirements.

@christian-herber-nxp
Copy link
Collaborator Author

I have done a rework of the spec to get it closer to what current specifications look like. Previously, it was in the style of RV64 & RVC, as this is where the encodings come from. Please have a look.
riscv-zilsd-v0.0.0.pdf
Built pdf attached for those who cannot reproduce easily.

christian-herber-nxp and others added 6 commits June 4, 2024 11:14
Signed-off-by: Christian Herber <[email protected]>
Added missing required extensions to the Makefile.
Pointed docs-resources at latest version.
Fixed heading levels in header.adoc.
- single page per instruction
- just moving information, no content change
@christian-herber-nxp christian-herber-nxp force-pushed the 13-clarifications-of-the-instructions-in-the-specification branch from b308e45 to 035e0cd Compare June 4, 2024 09:32
zilsd.adoc Outdated
{bits: 5, name: 'rd', type: 2, attr: ['5','dest≠0, dest[0]=0']},
{bits: 1, name: 'imm', type: 3, attr: ['1','offset[5]']},
{bits: 3, name: 'funct3', type: 8, attr: ['3','C.LDSP']},
Copy link
Collaborator

@tariqkurd-repo tariqkurd-repo Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the value of this funct3 isn't specified anywhere, and similarly for the other instructions.
and the ≠ character doesn't render in the pdf.
I really think the encoding page needs to specify every bit in the binary encoding, including things like width=D, otherwise you have to look in another doc to see what the encoding is (or riscv-opcodes)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have updated the encoding to explicit in a025144.
I have no idea why the ≠ character does not render.
It might be my local setup, because it is done in the same way for C and there it renders.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The not-equal sign was not rendering because the sans-serif font inclusion was commented out in the theme. I've updated docs-resources to fix this. I'll open an issue to update the docs-resources pointer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

zilsd.adoc Outdated Show resolved Hide resolved
- Not all instructions are generally reserved in RV32
- Zcf has an overlap with Zcmlsd in RV32
@christian-herber-nxp
Copy link
Collaborator Author

I hope this has had sufficient review. Thanks for the work on this. If you still spot an issue, please flag in a new issue.

@christian-herber-nxp christian-herber-nxp merged commit 6f657a2 into main Jun 4, 2024
1 check passed
@christian-herber-nxp christian-herber-nxp deleted the 13-clarifications-of-the-instructions-in-the-specification branch June 4, 2024 16:18
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.

Clarifications of the instructions in the specification
4 participants