[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 00/18] target-arm cleanup
From: |
Aurelien Jarno |
Subject: |
Re: [Qemu-devel] [PATCH 00/18] target-arm cleanup |
Date: |
Fri, 16 Oct 2009 19:56:19 +0200 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
On Fri, Oct 16, 2009 at 05:41:14PM +0200, Filip Navara wrote:
> 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
One of the fix was already in my branch (catched by Laurent Desnogues).
I have added the other fixes in my branch. The last to hunks are good
example why temp new/free should be done explicitly ;)
> > 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.
>
I'll merge that over the week-end if there are no more comments.
--
Aurelien Jarno GPG: 1024D/F1BCDB73
address@hidden http://www.aurel32.net