Re: [PATCH v3] Correct CRIS TCG register lifetime management

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Fri, Feb 19, 2021 at 11:53:48AM +0100, Stefan Sandström wrote:
> From: Stefan Sandstrom <stefans@axis.com>
> 
> Add and fix deallocation of temporary TCG registers in CRIS code
> generation.

Thanks Stefan,

There's still a couple of minor stylistic issues.

The Subject/Summary should be prefixed with the code area you're
changing. I'd suggest changing it

from:
Correct CRIS TCG register lifetime management
to:
target/cris: Plug leakage of TCG temporaries

We also try to avoid unrelated whitespace changes.
I've commented on the 2 I found inline.
Would be good if you could remove those in the next version.

Other than that, the patch looks good to me.
So, with those issues fixed, feel free to add the following tags:

Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Best regards,
Edgar

> 
> Change-Id: I17fce5d95bdc4418337ba885d53ba97afb1bafcc
> Signed-off-by: Stefan Sandström <stefans@axis.com>
> ---
>  target/cris/translate.c         | 127 +++++++++++++++++++++++---------
>  target/cris/translate_v10.c.inc |  70 ++++++++++++------
>  2 files changed, 138 insertions(+), 59 deletions(-)
> 
> diff --git a/target/cris/translate.c b/target/cris/translate.c
> index c893f877ab..2b35d818dd 100644
> --- a/target/cris/translate.c
> +++ b/target/cris/translate.c
> @@ -172,14 +172,21 @@ static int preg_sizes[] = {
>      tcg_gen_ld_tl(tn, cpu_env, offsetof(CPUCRISState, member))
>  #define t_gen_mov_env_TN(member, tn) \
>      tcg_gen_st_tl(tn, cpu_env, offsetof(CPUCRISState, member))
> +#define t_gen_movi_env_TN(member, c) \
> +    do { \
> +        TCGv tc = tcg_const_tl(c); \
> +        t_gen_mov_env_TN(member, tc); \
> +        tcg_temp_free(tc); \
> +    } while (0)
> +

Remove this extra blank line.


>  
>  static inline void t_gen_mov_TN_preg(TCGv tn, int r)
>  {
>      assert(r >= 0 && r <= 15);
>      if (r == PR_BZ || r == PR_WZ || r == PR_DZ) {
> -        tcg_gen_mov_tl(tn, tcg_const_tl(0));
> +        tcg_gen_movi_tl(tn, 0);
>      } else if (r == PR_VR) {
> -        tcg_gen_mov_tl(tn, tcg_const_tl(32));
> +        tcg_gen_movi_tl(tn, 32);
>      } else {
>          tcg_gen_mov_tl(tn, cpu_PR[r]);
>      }
> @@ -204,6 +211,8 @@ static inline void t_gen_mov_preg_TN(DisasContext *dc, int r, TCGv tn)
>      }
>  }
>  
> +
> +

Remove this unrelated blank lines.

Cheers,
Edgar