Re: [PATCH v1 4/5] target/riscv: progressively load the instruction during decode

["Alex Bennée" <alex.bennee@linaro.org>]

Robert Foley <robert.foley@linaro.org> writes:

> Hi,
> On Fri, 7 Feb 2020 at 10:01, Alex Bennée <alex.bennee@linaro.org> wrote:
>> -static void decode_RV32_64C0(DisasContext *ctx)
>> +static void decode_RV32_64C0(DisasContext *ctx, uint16_t opcode)
>>  {
>> -    uint8_t funct3 = extract32(ctx->opcode, 13, 3);
>> -    uint8_t rd_rs2 = GET_C_RS2S(ctx->opcode);
>> -    uint8_t rs1s = GET_C_RS1S(ctx->opcode);
>> +    uint8_t funct3 = extract32(opcode, 13, 3);
>
> We noticed that a uint16_t opcode is passed into this function and
> then passed on to extract32().
> This is a minor point, but the extract32() will validate the start and
> length values passed in according to 32 bits, not 16 bits.
> static inline uint32_t extract32(uint32_t value, int start, int length)
> {
>     assert(start >= 0 && length > 0 && length <= 32 - start);
> Since we have an extract32() and extract64(), maybe we could add an
> extract16() and use it here?

Yeah - I did consider if it would be worth adding a extract16 helper.
There are a fair number of places in the code base where uint16_t is
silently promoted to a uint32_t (and a couple where it is downcast).

I guess 16 bit instruction words are common enough we should support
them in the utils.

-- 
Alex Bennée