Re: [PATCH v7 09/14] target/riscv: rvk: add support for sha512 related instructions for RV32 in zknh extension

[Richard Henderson]

On 2/28/22 04:48, Weiwei Li wrote:
> +#define GEN_SHA512H_RV32(NAME, OP, NUM1, NUM2, NUM3) \
> +static void gen_##NAME(TCGv dest, TCGv src1, TCGv src2) \
> +{ \
> +    TCGv_i64 t0 = tcg_temp_new_i64(); \
> +    TCGv_i64 t1 = tcg_temp_new_i64(); \
> +    TCGv_i64 t2 = tcg_temp_new_i64(); \
> +    \
> +    tcg_gen_concat_tl_i64(t0, src1, src2); \
> +    tcg_gen_##OP##_i64(t1, t0, NUM1); \
> +    tcg_gen_concat_tl_i64(t2, src1, tcg_const_tl(0)); \

The bug here is tcg_const_tl instead of tcg_constant_tl, which leaks a 
temporary.

It's not the best option for zero-extension, though, as we don't optimize a deposit of 
zero like this (we probably should, but, hey).

Better would be

    tcg_gen_extu_tl_i64(t2, src1);
    tcg_gen_ext32u_i64(t2, t2);

Note that the first operation will *not* extend if TARGET_RISCV64, since it doesn't 
actually change type.  The second operation will be optimized away if TARGET_RISCV32, 
since the zero-extend has already happened.

BTW, it would be better to not use a large macro for this, and in the previous patch. 
Passing in parameters to a helper function would be easier to read and debug.


r~