Re: [PATCH 1/3] RISC-V: Fix doc for .insn

[Jim Wilson <jimw at sifive dot com>]

On Sun, May 26, 2019 at 11:37 PM Kito Cheng <kito@andestech.com> wrote:
> @@ -344,10 +344,10 @@ with the @samp{.insn} pseudo directive:
>  @item SB type: .insn sb opcode, func3, rd, rs1, symbol
>  @itemx SB type: .insn sb opcode, func3, rd, simm12(rs1)
>  @verbatim
> -+--------------+-----+-----+-------+-------------+-------------+
> -| simm21[11:5] | rs2 | rs1 | func3 | simm12[4:0] |      opcode |
> -+--------------+-----+-----+-------+-------------+-------------+
> -31             25    20    15      12            7             0
> ++------------+--------------+-----+-----+-------+-------------+-------------+--------+
> +| simm12[12] | simm12[10:5] | rs2 | rs1 | func3 | simm12[4:1] | simm12[11]] | opcode |
> ++------------+--------------+-----+-----+-------+-------------+-------------+--------+
> +31          30            25    20    15      12           7            0

Actually, on second thought, this looks a little confused.  The
testcase gas/testsuite/gas/riscv/insn.s uses sb for both stores and
branches.  If sb is a store, then the old description is correct.  if
sb is a branch, then the new description is correct.  But stores never
should have been using sb, they should be using s.  And the testcase
is using s for loads which is wrong, which prevents us from using s
for stores.  This looks like a bug in the original implementation
which we may not be able to fix without breaking backward
compatibility.  The .insn pseudo op is not used much, and probably not
used for loads and stores, so maybe it is OK to break this to get the
implementation correct.

Otherwise, the doc change looks correct, it just exposed a bug in the
implementation and the testcase.

Jim