RE: [PATCH v5 12/14] Hexagon (target/hexagon) Remove gen_log_predicated_reg_write[_pair]

[Taylor Simpson]

> -----Original Message-----
> From: Anton Johansson <anjo@rev.ng>
> Sent: Friday, February 24, 2023 6:06 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng; Brian Cain
> <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> <quic_mathbern@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 <anjo@rev.ng>