[RFC,05/13] target/riscv: Support UXL32 for shift instruction

Reuse 32-bit right shift instructions.

Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
---
 target/riscv/insn_trans/trans_rvi.c.inc | 12 ++++++++++++
 1 file changed, 12 insertions(+)

On 8/4/21 4:53 PM, LIU Zhiwei wrote:
>   static bool trans_srli(DisasContext *ctx, arg_srli *a)
>   {
> +    if (ctx->uxl32) {
> +        return trans_srliw(ctx, a);
> +    }
>       return gen_shifti(ctx, a, tcg_gen_shr_tl);
>   }

First, trans_srliw begins with REQUIRE_64BIT, which *should* fail when RV32 is in effect. 
  This means there's a missing change to is_32bit().

Second, the current decode for srli allows 7 bits of shift, while srilw only allows 5.  As 
a consequence, gen_shifti contains a check for TARGET_LONG_BITS and trans_srliw does not 
contain a check at all.  We need to diagnose an out-of-range shift for RV32 somewhere.

I recommend extending the gen_shift* family of helpers.

static bool gen_shifti_imm(DisasContext *ctx, arg_shift *a, int width,
                            void (*func)(TCGv, TCGv, target_long))
{
     TCGv dest, src1;

     if (a->shamt >= width) {
         return false;
     }
     dest = gpr_dst(ctx, a->rd);
     src1 = gpr_src(ctx, a->rs1);
     func(dest, src1, a->shamt);
     return true;
}

static bool gen_shifti(DisasContext *ctx, arg_shift *a, int width,
                        void (*func)(TCGv, TCGv, TCGv))
{...}

static void gen_srliw(TCGv dest, TCGv src1, target_long shamt)
{
     tcg_gen_extract_tl(dest, src1, shamt, 32 - shamt);
     tcg_gen_ext32s_tl(dest, dest);
}

static bool trans_srliw(DisasContext *ctx, arg_shift *a)
{
     REQUIRE_64BIT(ctx);
     return gen_shifti_imm(ctx, a, 32, gen_srliw);
}

static bool trans_srli(DisasContext *ctx, arg_shift *a)
{
     int xlen = is_32bit(ctx) ? 32 : 64;
     return gen_shifti_imm(ctx, a, xlen,
                           xlen == TARGET_LONG_BITS
                           ? tcg_gen_shri_tl : gen_srliw);
}

etc.  Perhaps that is_32bit() check above could be consolidated into some macro/inline.


r~

On 2021/8/6 上午6:17, Richard Henderson wrote:
> On 8/4/21 4:53 PM, LIU Zhiwei wrote:
>>   static bool trans_srli(DisasContext *ctx, arg_srli *a)
>>   {
>> +    if (ctx->uxl32) {
>> +        return trans_srliw(ctx, a);
>> +    }
>>       return gen_shifti(ctx, a, tcg_gen_shr_tl);
>>   }
>
> First, trans_srliw begins with REQUIRE_64BIT, which *should* fail when 
> RV32 is in effect.  This means there's a missing change to is_32bit().

As I have replied in another patch, ctx->uxl32 already indicats 64 bit 
CPU.  Anyway, I will think more about how to merge is_32bit() and uxl32 
in next patch set.

>
> Second, the current decode for srli allows 7 bits of shift, while 
> srilw only allows 5.  As a consequence, gen_shifti contains a check 
> for TARGET_LONG_BITS and trans_srliw does not contain a check at all.  
> We need to diagnose an out-of-range shift for RV32 somewhere.
>
Yes, it's not proper directly use *w here.  Fix it in next patch set.

Zhiwei

> I recommend extending the gen_shift* family of helpers.
>
> static bool gen_shifti_imm(DisasContext *ctx, arg_shift *a, int width,
>                            void (*func)(TCGv, TCGv, target_long))
> {
>     TCGv dest, src1;
>
>     if (a->shamt >= width) {
>         return false;
>     }
>     dest = gpr_dst(ctx, a->rd);
>     src1 = gpr_src(ctx, a->rs1);
>     func(dest, src1, a->shamt);
>     return true;
> }
>
> static bool gen_shifti(DisasContext *ctx, arg_shift *a, int width,
>                        void (*func)(TCGv, TCGv, TCGv))
> {...}
>
> static void gen_srliw(TCGv dest, TCGv src1, target_long shamt)
> {
>     tcg_gen_extract_tl(dest, src1, shamt, 32 - shamt);
>     tcg_gen_ext32s_tl(dest, dest);
> }
>
> static bool trans_srliw(DisasContext *ctx, arg_shift *a)
> {
>     REQUIRE_64BIT(ctx);
>     return gen_shifti_imm(ctx, a, 32, gen_srliw);
> }
>
> static bool trans_srli(DisasContext *ctx, arg_shift *a)
> {
>     int xlen = is_32bit(ctx) ? 32 : 64;
>     return gen_shifti_imm(ctx, a, xlen,
>                           xlen == TARGET_LONG_BITS
>                           ? tcg_gen_shri_tl : gen_srliw);
> }
>
> etc.  Perhaps that is_32bit() check above could be consolidated into 
> some macro/inline.
>
>
> r~