Re: [Qemu-devel] [PATCH v3 24/35] target/riscv: Move gen_arith_imm() decoding into trans_* functions

[Richard Henderson]

On 10/31/18 1:20 PM, Bastian Koppelmann wrote:
>  static bool trans_slli(DisasContext *ctx, arg_slli *a)
>  {
> -    gen_arith_imm(ctx, OPC_RISC_SLLI, a->rd, a->rs1, a->shamt);
> +    if (a->rd != 0) {
> +        TCGv t = tcg_temp_new();
> +        gen_get_gpr(t, a->rs1);
> +
> +        if (a->shamt >= TARGET_LONG_BITS) {
> +            return false;
> +        }
> +        tcg_gen_shli_tl(t, t, a->shamt);
> +
> +        gen_set_gpr(a->rd, t);
> +        tcg_temp_free(t);
> +    } /* NOP otherwise */
>      return true;
>  }
>  
>  static bool trans_srli(DisasContext *ctx, arg_srli *a)
>  {
> -    gen_arith_imm(ctx, OPC_RISC_SHIFT_RIGHT_I, a->rd, a->rs1, a->shamt);
> +    if (a->rd != 0) {
> +        TCGv t = tcg_temp_new();
> +        gen_get_gpr(t, a->rs1);
> +        tcg_gen_shri_tl(t, t, a->shamt);
> +        gen_set_gpr(a->rd, t);
> +        tcg_temp_free(t);
> +    } /* NOP otherwise */
>      return true;
>  }
>  
>  static bool trans_srai(DisasContext *ctx, arg_srai *a)
>  {
> -    gen_arith_imm(ctx, OPC_RISC_SHIFT_RIGHT_I, a->rd, a->rs1, a->shamt | 
> 0x400);
> +    if (a->rd != 0) {
> +        TCGv t = tcg_temp_new();
> +        gen_get_gpr(t, a->rs1);
> +        tcg_gen_sari_tl(t, t, a->shamt);
> +        gen_set_gpr(a->rd, t);
> +        tcg_temp_free(t);
> +    } /* NOP otherwise */
>      return true;
>  }

Surely the shri and sari functions need the same shamt >= TARGET_LONG_BITS
check as slli.  Otherwise RV32 shri should definitely produce an assert in
tcg_gen_shri_tl.

I did wonder about changing the decode of the shift functions such that only
the top two bits of the imm are reserved for secondary parsing of the shift
type, and the other 10 bits are passed down into trans_foo.  At which point the
TARGET_LONG_BITS check takes care of other illegalities.

Which means that the parsing for slli and slliw are identical, and also that
for the far future when RV128 is a thing, we don't have to change the parsing.


r~