Robert Foley <robert.fo...@linaro.org> writes:
> Hi,
> On Fri, 7 Feb 2020 at 10:01, Alex Bennée <alex.ben...@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
