Bug 26051

Summary: [RISCV] .insn documentation update
Product: binutils Reporter: Frédéric Pétrot <frederic.petrot>
Component: gasAssignee: Jim Wilson <wilson>
Severity: enhancement CC: wilson
Priority: P2    
Version: 2.34   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed: 2020-06-01 00:00:00
Attachments: diff that patches the doc
my proposed fix

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>:


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.
            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 !