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

[Aurelien Jarno]

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