From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Myovj-0003bZ-Do for qemu-devel@nongnu.org; Fri, 16 Oct 2009 11:41:23 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Myove-0003ZQ-1h for qemu-devel@nongnu.org; Fri, 16 Oct 2009 11:41:22 -0400 Received: from [199.232.76.173] (port=36334 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Myovd-0003Z2-RR for qemu-devel@nongnu.org; Fri, 16 Oct 2009 11:41:17 -0400 Received: from mail-ew0-f211.google.com ([209.85.219.211]:48442) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Myovc-0002by-T4 for qemu-devel@nongnu.org; Fri, 16 Oct 2009 11:41:17 -0400 Received: by ewy7 with SMTP id 7so1600164ewy.34 for ; Fri, 16 Oct 2009 08:41:15 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20091015174054.GZ4127@hall.aurel32.net> References: <5b31733c0907190843k11ead885tc6b4c81b79f7257b@mail.gmail.com> <20091015174054.GZ4127@hall.aurel32.net> Date: Fri, 16 Oct 2009 17:41:14 +0200 Message-ID: <5b31733c0910160841r60c16a57pf90861bd1d6e739@mail.gmail.com> Subject: Re: [Qemu-devel] [PATCH 00/18] target-arm cleanup From: Filip Navara Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aurelien Jarno Cc: Laurent Desnogues , qemu-devel , Paul Brook Hi! On Thu, Oct 15, 2009 at 7:40 PM, Aurelien Jarno wrot= e: [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: > > | =A0 =A0if (rn =3D=3D 15) { > | =A0 =A0 =A0 =A0tmp =3D new_tmp(); > | =A0 =A0 =A0 =A0tcg_gen_movi_i32(tmp, 0); > | =A0 =A0} else { > | =A0 =A0 =A0 =A0tmp =3D load_reg(s, rn); > | =A0 =A0} > > ... > > | =A0 =A0if (rd !=3D 15) { > | =A0 =A0 =A0 =A0store_reg(s, rd, tmp); > | =A0 =A0} else { > | =A0 =A0 =A0 =A0dead_tmp(tmp); > | =A0 =A0} > > 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=E4ki . A fix is available at http://repo.or.cz/w/qemu/navara.git?a=3Dcommit;h=3D2ce696baa6fc5d99522cf387= b6a4913807fd43ed > 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