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

Add .insn directive #106

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add .insn directive #106

wants to merge 1 commit into from

Conversation

Timmmm
Copy link
Contributor

@Timmmm Timmmm commented Jun 26, 2024

Add .insn directive with brief description and link to full documentation.

Copy link
Collaborator

@cmuellner cmuellner left a comment

Choose a reason for hiding this comment

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

Is this available in GCC and LLVM?

@asb
Copy link
Contributor

asb commented Jun 26, 2024

Is this available in GCC and LLVM?

It is.

@Timmmm perhaps worth giving an example using the named opcode as well (e.g. .insn r MADD, 0, 0, fa0, fa1, fa2, fa3).

Add .insn directive with brief description and link to full documentation.

Signed-off-by: Tim Hutt <[email protected]>
@Timmmm
Copy link
Contributor Author

Timmmm commented Jun 27, 2024

I added .insn r OP, 0, 0, a0, a1, a2 so it's the same instruction as the other examples.


* `.insn value` - emit a raw instruction with the given value
* `.insn insn_length, value` - the same, but also verify that the instruction length has the given value in bytes
* `.insn type, operand [,...,operand_n]` - use one of the [common instruction formats](https://sourceware.org/binutils/docs/as/RISC_002dV_002dFormats.html)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like having a reference to external (non-final) documentation in the normative part of the document because it makes this manual no longer self-contained.

Also, the referenced page seems to be incomplete:

For the complete list of all instruction format variants see The RISC-V Instruction Set Manual Volume I: User-Level ISA.

I think completeness is not necessary (we probably don't want to update this document whenever a new instruction format is specified). So, probably the best way to document this is to describe the mechanics of this format (opcode fields, function fields, register fields, immediates/symbols), where to lookup the names in the ISA manual and a few examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like a lot of work! Can I just remove the link for now and say - use one of the standard instruction formats described in the ISA manual? Not fully documented but still better than it was before.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Timmmm I have used the documentation from the gnu-as docs and created a commit to add them here. Would you like to cherry-pick my change onto your branch so that these commits can go in at the same time? charlie-rivos@eebc39b

Copy link
Collaborator

Choose a reason for hiding this comment

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

@charlie-rivos, this looks great! And it is certainly more than I asked for.

@Timmmm, I did not want to cause much work, but I failed to express my intent well. All instruction formats strictly follow the instruction formats in the ISA spec. So, all that needs to be done is to explain the possible part of an instruction format and how to come from the ISA spec to the asm directive.

I was thinking of something like this:

The instruction formats of the .insn directive follow the instruction format
definition in the RISC-V ISA specification.

The generic form is as follows:
  .insn <type> <fields>

<type> is the instruction type (e.g. r, i, s, or cj).
These types are specified in the RISC-V ISA specification.

<fields> is a comma-separated list of the instruction fields.
The order of the fields is achieved by grouping them and listing
them from LSB to MSB. The groups are:
* opcode fields
* function fields
* register fields
* immediates and symbols

E.g. an instruction with the fields (sorted from LSB to MSB):
  opcode7, rd, func3, rs1, rs2, func7
Gets listed as follows:
  opcode7, func3, func7, rd, rs1, rs2

For more examples, refer to the Binutils documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmuellner Ah I see, I did not know it worked like that! Is that grouping documented somewhere already or is it just a convention that binutils decided that we can document?

I'll do an update integrating both of your changes, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

The operands and their order are defined in Binutils (see riscv_insn_types in opcodes/riscv-opc.c).
I don't think that the order is documented anywhere and that Binutils contributors decided that format.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cmuellner Should I open a PR with my change? I was thinking that @Timmmm would apply the changes but am happy to open one myself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I haven't had time so go for it!

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.

4 participants