Re: [PATCH v3 08/14] tcg/riscv: Implement vector cmp ops

[Richard Henderson]

On 9/4/24 07:27, LIU Zhiwei wrote:
> @@ -2322,6 +2411,51 @@ static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
>           riscv_set_vec_config_vl(s, type);
>           tcg_out_opc_vi(s, OPC_VXOR_VI, a0, a1, -1, true);
>           break;
> +    case INDEX_op_cmpsel_vec:
> +        TCGArg a3, a4;
> +        int c3, c4;
> +        TCGCond cond;

While I suppose this compiles, it's not great to have new variables added randomly within 
a switch.  At minimum, add { } around the block, but consider breaking out a separate 
tcg_out_cmpsel function, akin to tcg_out_movcond et al.

> @@ -2332,10 +2466,27 @@ static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
>   void tcg_expand_vec_op(TCGOpcode opc, TCGType type, unsigned vece,
>                          TCGArg a0, ...)
>   {
> +    va_list va;
> +    TCGv_vec v0, v1;
> +    TCGArg a2, a3;
> +
> +    va_start(va, a0);
> +    v0 = temp_tcgv_vec(arg_temp(a0));
> +    v1 = temp_tcgv_vec(arg_temp(va_arg(va, TCGArg)));
> +    a2 = va_arg(va, TCGArg);
> +
>       switch (opc) {
> +    case INDEX_op_cmp_vec:
> +        a3 = va_arg(va, TCGArg);
> +        vec_gen_6(INDEX_op_cmpsel_vec, type, vece, tcgv_vec_arg(v0),
> +                    tcgv_vec_arg(v1), a2,
> +                    tcgv_i64_arg(tcg_constant_i64(-1)),
> +                    tcgv_i64_arg(tcg_constant_i64(0)), a3);
> +        break;
>       default:
>           g_assert_not_reached();
>       }
> +    va_end(va);
>   }

Better to use "TCGArg a0, a1".  Converting through arg_tmp + temp_tcgv_vec to v0/v1 and 
then undoing that with tcgv_vec_arg is confusing.


r~