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

[Filip Navara]

Hi!

On Thu, Oct 15, 2009 at 7:40 PM, Aurelien Jarno <address@hidden> wrote:
[snip]
> 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.

I'll make sure not to mess it up next time.

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

Neither do I, but removing that altogether would make the patch series
way too big.

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

Fully agreed.

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

Yep.

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

Apart from the points you have raised about specific patches there
were few more minor bugs in the series spotted by Juha Riihimäki
<address@hidden>. A fix is available at
http://repo.or.cz/w/qemu/navara.git?a=commit;h=2ce696baa6fc5d99522cf387b6a4913807fd43ed

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

I'd be glad if the series gets applied, provided that your fixes and
the one referenced above is included. If necessary I can resend the
whole series with the fixes included, but I'd rather avoid it.

Best regards,
Filip Navara