Re: [PATCH v3 13/21] target/riscv: support for 128-bit shift instructions

[Richard Henderson]

On 10/19/21 2:48 AM, Frédéric Pétrot wrote:
> +    } else {
> +        TCGv src1l = get_gpr(ctx, a->rs1, ext),
> +             src1h = get_gprh(ctx, a->rs1),
> +             destl = tcg_temp_new(),
> +             desth = tcg_temp_new();

Don't do this comma, reuse of type and indent thing.
I know there are several instances.

> +        if (max_len < 128) {
> +            func(destl, src1l, a->shamt);
> +            gen_set_gpr(ctx, a->rd, destl);
> +            gen_set_gprh(ctx, a->rd, desth);

You hadn't initialized desth.  Again, where gen_set_gpr and gen_set_gpr128 are clearer 
than this.

>       int olen = get_olen(ctx);
>       if (olen != TARGET_LONG_BITS) {
>           if (olen == 32) {
>               f_tl = f_32;
> -        } else {
> +        } else if (olen != 128) {
>               g_assert_not_reached();
>           }
>       }
> -    return gen_shift_imm_fn(ctx, a, ext, f_tl);
> +    return gen_shift_imm_fn(ctx, a, ext, f_tl, f_128);

Surely it would be cleaner to split out f_128 at this point, and not pass along f_128 to 
gen_shift_imm_fn?

>   static bool gen_shift(DisasContext *ctx, arg_r *a, DisasExtend ext,
> -                      void (*func)(TCGv, TCGv, TCGv))
> +                      void (*func)(TCGv, TCGv, TCGv),
> +                      void (*f128)(TCGv, TCGv, TCGv, TCGv, TCGv))
>   {
> -    TCGv dest = dest_gpr(ctx, a->rd);
> -    TCGv src1 = get_gpr(ctx, a->rs1, ext);
>       TCGv src2 = get_gpr(ctx, a->rs2, EXT_NONE);
>       TCGv ext2 = tcg_temp_new();
>   
>       tcg_gen_andi_tl(ext2, src2, get_olen(ctx) - 1);
> -    func(dest, src1, ext2);
>   
> -    gen_set_gpr(ctx, a->rd, dest);
> +    if (get_xl_max(ctx) < MXL_RV128) {
> +        TCGv dest = dest_gpr(ctx, a->rd);
> +        TCGv src1 = get_gpr(ctx, a->rs1, ext);
> +        func(dest, src1, ext2);
> +
> +        gen_set_gpr(ctx, a->rd, dest);
> +    } else {
> +        TCGv src1l = get_gpr(ctx, a->rs1, ext),
> +             src1h = get_gprh(ctx, a->rs1),
> +             destl = tcg_temp_new(),
> +             desth = tcg_temp_new();

Should be dest_gpr*.

> +
> +        if (get_olen(ctx) < 128) {
> +            func(destl, src1l, ext2);
> +            gen_set_gpr(ctx, a->rd, destl);
> +            gen_set_gprh(ctx, a->rd, desth);
> +        } else {
> +            assert(f128 != NULL);

I think you don't want to assert, but just return false.  This will make all of the Zb 
instructions come out undefined for rv128, which is probably what you want.  You'd want to 
do that earlier, before all the get_gpr* above.

> @@ -447,9 +486,75 @@ static bool trans_sub(DisasContext *ctx, arg_sub *a)
>       return gen_arith(ctx, a, EXT_NONE, tcg_gen_sub_tl);
>   }
>   
> +enum M128_DIR {
> +    M128_LEFT,
> +    M128_RIGHT,
> +    M128_RIGHT_ARITH
> +};

Why "M"?

> +         cnst_zero = tcg_constant_tl(0);

This is ctx->zero.

Lots of instances throughout your patch set
though this is the first time I noticed.

> +    tcg_gen_setcondi_tl(TCG_COND_GEU, tmp1, arg2, 64);

You should fold this test into the movcond.

> +        tcg_gen_movi_tl(tmp, 64);
> +        tcg_gen_sub_tl(tmp, tmp, shamt);

tcg_gen_subfi_tl.

The indentation is off in gen_sll_i128.

Hmm.  3 * (and + shift + cmp + cmov) + 2 * (sub + or) = 16 ops.
Not horrible...

Let's see.

    ls = sh & 63;        1
    rs = -sh & 63;       3
    hs = sh & 64;        4

    ll = s1l << ls;      5
    h0 = s1h << ls;      6
    lr = s1l >> rs;      7
    h1 = h0 | lr;        8

    dl = hs ? 0 : ll;    10
    dh = hs ? ll : h1;   12

That seems right, and would be 4 ops smaller.
Would need testing of course.


r~