Bug 26051 - [RISCV] .insn documentation update
Summary: [RISCV] .insn documentation update
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.34
: P2 enhancement
Target Milestone: ---
Assignee: Jim Wilson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-27 06:48 UTC by Frédéric Pétrot
Modified: 2020-06-17 11:44 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2020-06-01 00:00:00


Attachments
diff that patches the doc (422 bytes, patch)
2020-05-27 06:48 UTC, Frédéric Pétrot
Details | Diff
my proposed fix (677 bytes, patch)
2020-06-01 23:00 UTC, Jim Wilson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Pétrot 2020-05-27 06:48:34 UTC
Created attachment 12571 [details]
diff that patches the doc

Make documentation in line with code in riscv-opc.c for the i, s and b/sb types of the .insn directive.
Comment 1 Jim Wilson 2020-06-01 22:59:36 UTC
+@itemx I type for loads: .insn i opcode, func3, rd, simm12(rs1)

The .insn parsing code doesn't know whether we have a load or not, so this syntax is actually valid both for loads and non-loads, likewise with the other syntax.  We should include this line, but I think the "for loads" part should be dropped.  None of the other descriptions have similar text mentioning instruction type.

+@item S type: .insn s opcode, func3, rs2, simm12(rs1)

This looks right.  For completeness we might want an alternate syntax that accepts rs1 and simm12 as separate operands, as someone might want to use the S type for a non-store, but no one has asked for that so I'm not worrying about that for now.

+@item SB type: .insn sb opcode, func3, rs1, rs2, symbol
+@itemx SB type: .insn sb opcode, func3, rs1, rs2, simm12
+@itemx B type: .insn b opcode, func3, rs1, rs2, symbol
+@itemx B type: .insn b opcode, func3, rs1, rs1, simm12

The .insn parsing code doesn't allow the simm12 syntax here.  Only pc-relative symbols.  So these lines should be dropped.  You are right about changing the regs from rd/rs1 to rs1/rs2.  There is a typo on the last line with two rs1s, but that line needs to be dropped anyways.  Since we are modifying this, I think we should promote B to @item and demote SB to @itemx, as B is the current name and SB is the old name for this insn format.

Similarly we should promote J and demote UJ.

This patch is small enough and obvious enough that I believe we can accept it without a copyright assignment.  In general, if you want to contribute to GNU packages, you need to go through the copyright assignment paperwork.  Otherwise, it is better to describe the change you want rather than send a patch.
Comment 2 Jim Wilson 2020-06-01 23:00:35 UTC
Created attachment 12579 [details]
my proposed fix
Comment 3 Jim Wilson 2020-06-01 23:00:55 UTC
I added my proposed fix as an attachment.
Comment 4 cvs-commit@gcc.gnu.org 2020-06-03 01:37:46 UTC
The master branch has been updated by Jim Wilson <wilson@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=bb7322c67111024f5977deb85abd777ec713b1a9

commit bb7322c67111024f5977deb85abd777ec713b1a9
Author: Jim Wilson <jimw@sifive.com>
Date:   Tue Jun 2 18:36:14 2020 -0700

    RISC-V: Fix minor bugs in .insn docs.
    
    This fixes some minor bugs in the docs for the .insn directive pointed out
    by Frédéric Pétrot, and I added a few more cleanups since I was changing
    the docs.
    
            gas/
            PR 26051
            * doc/c-riscv.texi (RISC-V-Formats): Add missing I format using
            simm12(rs1).  Correct S format to use simm12(rs1).  Drop SB and B
            formats using simm12(rs1).  Correct SB and B to use rs1 and rs2.
            Move B before SB.  Move J before UJ.
Comment 5 Jim Wilson 2020-06-03 01:39:02 UTC
Fixed on mainline.
Comment 6 Frédéric Pétrot 2020-06-17 11:44:47 UTC
Thanks Jim !