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

[Stefan Sandström]

Hi Edgar,

> On 19 Feb 2021, at 12:19, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> 
> 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

OK, makes sense.

> 
> 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.

Oops, sorry about that.
I've posted v4 of the patch that fixes the subject and white-spaces.

Thanks,
-stefan

> 
> 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