> -----Original Message-----
> From: Anton Johansson <a...@rev.ng>
> Sent: Friday, February 24, 2023 6:06 AM
> To: Taylor Simpson <tsimp...@quicinc.com>; qemu-devel@nongnu.org
> Cc: richard.hender...@linaro.org; phi...@linaro.org; a...@rev.ng; Brian Cain
> <bc...@quicinc.com>; Matheus Bernardino (QUIC)
> <quic_mathb...@quicinc.com>
> Subject: Re: [PATCH v5 12/14] Hexagon (target/hexagon) Remove
> gen_log_predicated_reg_write[_pair]
>
> On 1/31/23 23:56, Taylor Simpson wrote:
> > static void gen_log_reg_write_pair(int rnum, TCGv_i64 val)
> > {
> > const target_ulong reg_mask_low = reg_immut_masks[rnum]; @@
> > -167,6 +120,7 @@ static void gen_log_reg_write_pair(int rnum, TCGv_i64
> val)
> > }
> >
> > tcg_temp_free(val32);
> > + tcg_temp_free_i64(val);
> > }
> >
> > void gen_log_pred_write(DisasContext *ctx, int pnum, TCGv val) @@
> > -306,12 +260,14 @@ static inline void
> gen_write_ctrl_reg_pair(DisasContext *ctx, int reg_num,
> > TCGv_i64 val)
> > {
> > if (reg_num == HEX_REG_P3_0_ALIASED) {
> > + TCGv result = get_result_gpr(ctx, reg_num + 1);
> > TCGv val32 = tcg_temp_new();
> > tcg_gen_extrl_i64_i32(val32, val);
> > gen_write_p3_0(ctx, val32);
> > tcg_gen_extrh_i64_i32(val32, val);
> > - gen_log_reg_write(reg_num + 1, val32);
> > + tcg_gen_mov_tl(result, val32);
> > tcg_temp_free(val32);
> > + tcg_temp_free_i64(val);
> > } else {
> > gen_log_reg_write_pair(reg_num, val);
> > if (reg_num == HEX_REG_QEMU_PKT_CNT) {
>
> Hiding the free of a TCGv argument scares me a bit, it's bound to cause
> annoying bugs down the line, I feel.
>
> Any reason we can't keep the frees in the same scope as allocation?
The only ones that need to be free'd are the pairs. Those are created by
get_result_gpr_pair, so I'm using the gen_log_reg_write_pair function as the
central place to free them.
Also, Richard Henderson is working on a patch series that will eliminate the
need for all free's 😊
>
> Otherwise, the patch looks good,
>
> Reviewed-by: Anton Johansson <a...@rev.ng>
