qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Filip Navara <filip.navara@gmail.com>
To: Aurelien Jarno <aurelien@aurel32.net>
Cc: Laurent Desnogues <laurent.desnogues@gmail.com>,
	qemu-devel <qemu-devel@nongnu.org>,
	Paul Brook <paul@codesourcery.com>
Subject: Re: [Qemu-devel] [PATCH 00/18] target-arm cleanup
Date: Fri, 16 Oct 2009 17:41:14 +0200	[thread overview]
Message-ID: <5b31733c0910160841r60c16a57pf90861bd1d6e739@mail.gmail.com> (raw)
In-Reply-To: <20091015174054.GZ4127@hall.aurel32.net>

Hi!

On Thu, Oct 15, 2009 at 7:40 PM, Aurelien Jarno <aurelien@aurel32.net> 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
<juha.riihimaki@nokia.com>. 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

  parent reply	other threads:[~2009-10-16 15:41 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-07-19 15:43 [Qemu-devel] [PATCH 00/18] target-arm cleanup Filip Navara
2009-10-15 17:40 ` Aurelien Jarno
2009-10-15 17:49   ` Laurent Desnogues
2009-10-15 18:36     ` Aurelien Jarno
2009-10-16 15:41   ` Filip Navara [this message]
2009-10-16 17:56     ` Aurelien Jarno
2009-10-19  6:23       ` Juha.Riihimaki
2009-10-19  6:35         ` Laurent Desnogues
2009-11-10 23:38           ` Paul Brook
2009-11-10 23:53             ` Aurelien Jarno
2009-10-19 13:23         ` Aurelien Jarno
2009-10-20  6:18           ` Juha.Riihimaki
2009-11-10 23:39   ` Paul Brook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5b31733c0910160841r60c16a57pf90861bd1d6e739@mail.gmail.com \
    --to=filip.navara@gmail.com \
    --cc=aurelien@aurel32.net \
    --cc=laurent.desnogues@gmail.com \
    --cc=paul@codesourcery.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).