Re: [Qemu-devel] [PATCH 00/18] target-arm cleanup

[Aurelien Jarno]

On Sun, Jul 19, 2009 at 05:43:03PM +0200, Filip Navara wrote:
> Hi,
Hi,

> the set of patches I just sent are intended to improve the TCG
> register allocation in target-arm code. The goals are:
> 
> - Use TCG memory-backed variables for the CPU registers
> - Get rid of cpu_T variables
> - Remove unused code
> 
> Several emulation correctness bugs are also fixed by the patches, but
> I couldn't verify if the fixes actually work (VUZP and RFE
> instructions).
> 

I have just reviewed this series of patches, you've done a good job! I
have only a few minor comments.

First of all on the code style. It's nice for patches that only affect
one target to mention that in the title of the patch, for example by
prefixing it with "target-arm". Also a few patches (06, 08, 11, 12, 13 
and 15) have indentation issues (tabs instead of spaces) and/or dead 
spaces at the end of lines. It's something I have fixed on my local tree,
no need to resend the patches for that.

On the code itself, I don't really like the remaining, new_tmp(),
dead_tmp(), and even more the fact that some functions can allocate
(e.g load_reg) or free (e.g. store_reg) some TCG variables implicitely.
This is a way to make errors by reallocating or forgetting to free some
variables, and that leads to strange code like:

|    if (rn == 15) {
|        tmp = new_tmp();
|        tcg_gen_movi_i32(tmp, 0);
|    } else {
|        tmp = load_reg(s, rn);
|    }

...

|    if (rd != 15) {
|        store_reg(s, rd, tmp);
|    } else {
|        dead_tmp(tmp);
|    }

Also I am not sure that counting the number of allocated temps has its
place in target-arm/translate.c. It should probably be moved to
tcg/tcg.c and enabled only when CONFIG_DEBUG_TCG is defined, so that it
can benefits all targets.

On the other hand, I fully understand that this code results from
incremental changes from the current code. It should probably be fixed
by an additional patch to the whole series.

Otherwise, I have no specific comments to the individual patches.

As the minor issue concerning TCG temp variables can be fixed later by
a separate patch, and that it is not a regression from the current code,
I would like, if nobody opposes, to apply this whole series (including
my local white spaces fixes).

Regards,
Aurelien

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net