Re: [PATCH v2 1/3] target/riscv: Set the opcode in DisasContext

[Richard Henderson <richard.henderson@linaro.org>]

On 9/8/21 6:54 AM, Alistair Francis wrote:
> From: Alistair Francis <alistair.francis@wdc.com>
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   target/riscv/translate.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index e356fc6c46..25670be435 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -485,20 +485,20 @@ static uint32_t opcode_at(DisasContextBase *dcbase, target_ulong pc)
>   /* Include the auto-generated decoder for 16 bit insn */
>   #include "decode-insn16.c.inc"
>   
> -static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t opcode)
> +static void decode_opc(CPURISCVState *env, DisasContext *ctx)
>   {
>       /* check for compressed insn */
> -    if (extract16(opcode, 0, 2) != 3) {
> +    if (extract16(ctx->opcode, 0, 2) != 3) {
>           if (!has_ext(ctx, RVC)) {
>               gen_exception_illegal(ctx);
>           } else {
>               ctx->pc_succ_insn = ctx->base.pc_next + 2;
> -            if (!decode_insn16(ctx, opcode)) {
> +            if (!decode_insn16(ctx, ctx->opcode)) {
>                   gen_exception_illegal(ctx);
>               }
>           }
>       } else {
> -        uint32_t opcode32 = opcode;
> +        uint32_t opcode32 = ctx->opcode;
>           opcode32 = deposit32(opcode32, 16, 16,
>                                translator_lduw(env, ctx->base.pc_next + 2));

You needed to write back to ctx->opcode here.

I think that all of the other changes are less than ideal -- let the value stay in a 
register as long as possible and drop them to memory immediately before calling 
decode_insn{16,32}, just before the write to pc_succ_insn in both cases.


r~


>           ctx->pc_succ_insn = ctx->base.pc_next + 4;
> @@ -561,9 +561,9 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
>   {
>       DisasContext *ctx = container_of(dcbase, DisasContext, base);
>       CPURISCVState *env = cpu->env_ptr;
> -    uint16_t opcode16 = translator_lduw(env, ctx->base.pc_next);
> +    ctx->opcode = translator_lduw(env, ctx->base.pc_next);
>   
> -    decode_opc(env, ctx, opcode16);
> +    decode_opc(env, ctx);
>       ctx->base.pc_next = ctx->pc_succ_insn;
>       ctx->w = false;
>   
>