[Qemu-devel] RISC-V: insn32.decode: Confusing encodings

[Qemu-devel] RISC-V: insn32.decode: Confusing encodings

[Maxim Blinov]

Hi all,

I've been going through the insn32.decode file, and found some
confusing inconsistencies with the ISA spec that I don't understand. I
hope some of you can clarify.

There is a field defined called "%sh10" as follows:
%sh10    20:10

Which is used in the "@sh" format as follows:
@sh ...... ...... .....  ... ..... ....... &shift  shamt=%sh10     %rs1 %rd

And the "@sh" format specifier is used for the following rv32i
instruction defs:

slli     00.... ......    ..... 001 ..... 0010011 @sh
srli     00.... ......    ..... 101 ..... 0010011 @sh
srai     01.... ......    ..... 101 ..... 0010011 @sh

First question: Why does the %sh10 field exist? There are no 10-bit
shamt fields anywhere in the spec.

Second question: For rv32i, "SLLI" is defined as follows in the spec:

0000000 shamt[4:0] rs1[4:0] 001 rd[4:0] 0010011  |  SLLI

That is, the first 7 bits *must* be zero. So why does the QEMU
definition above only specify the first 2 bits, and treat the next 10
bits as a so-called "sh10" field? Surely that shouldn't work and will
catch false instructions right? And even if it does work, surely we
would want an explicit definition, something more like

%sh5    20:5
@sh ...... ...... .....  ... ..... ....... &shift  shamt=%sh5     %rs1 %rd

slli     0000000 .....    ..... 001 ..... 0010011 @sh
srli     0000000 .....    ..... 101 ..... 0010011 @sh
srai     0100000 .....    ..... 101 ..... 0010011 @sh

Another thing I noticed is that the rv64i ISA redefines the slli, srli
and srai encodings by stealing a bit from the immediate field, like
so:

000000 shamt[5:0] rs1[4:0] 001 rd[4:0] 0010011  |  SLLI

Consider the case that we have a 32 bit cpu and we wanted to have a
custom instruction encoded like so:

      |This bit set
      v
0000001 shamt[4:0] rs1[4:0] 001 rd[4:0] 0010011  |  MY_INSN

In 64 bit risc-v, we can't have that instruction because that bit is
used in the shift field for the SLLI instruction.  But it should be
fine to use in 32-bit risc-v.

There are two files currently: insn32.decode, and insn32-64.decode.
The insn32-64.decode file is additive, but some instructions as simply
encoded differently in 64 bit mode.

Why not have two separate insn32.decode and insn64.decode files?

I hope I'm understanding the ISA correctly...

Maxim

Re: [Qemu-devel] RISC-V: insn32.decode: Confusing encodings

[Richard Henderson]

On 8/6/19 5:48 AM, Maxim Blinov wrote:
> slli     00.... ......    ..... 001 ..... 0010011 @sh
> srli     00.... ......    ..... 101 ..... 0010011 @sh
> srai     01.... ......    ..... 101 ..... 0010011 @sh
> 
> First question: Why does the %sh10 field exist? There are no 10-bit
> shamt fields anywhere in the spec.
> 
> Second question: For rv32i, "SLLI" is defined as follows in the spec:
> 
> 0000000 shamt[4:0] rs1[4:0] 001 rd[4:0] 0010011  |  SLLI

Bits [9:5] of the field are checked against zero later, with

    if (a->shamt >= TARGET_LONG_BITS) {
        return false;
    }

It was done this way to be compatible between rv32, rv64, and a future rv128.
Which I admit would only need 7 bits not 10, but it didn't seem to matter
either way.

> Consider the case that we have a 32 bit cpu and we wanted to have a
> custom instruction encoded like so:
> 
>       |This bit set
>       v
> 0000001 shamt[4:0] rs1[4:0] 001 rd[4:0] 0010011  |  MY_INSN
> 
> In 64 bit risc-v, we can't have that instruction because that bit is
> used in the shift field for the SLLI instruction.  But it should be
> fine to use in 32-bit risc-v.

Ah, well, for that you would in fact need to adjust the decode files.

I do question why you'd want to define MY_INSN in such a way as to be
incompatible with an rv64 implementation.  Why not place your new bit higher in
the field?

> Why not have two separate insn32.decode and insn64.decode files?

To avoid unnecessary duplication, of course.


r~