From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MyURx-00084D-RX for qemu-devel@nongnu.org; Thu, 15 Oct 2009 13:49:17 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MyURw-00082E-Jk for qemu-devel@nongnu.org; Thu, 15 Oct 2009 13:49:16 -0400 Received: from [199.232.76.173] (port=46797 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MyURw-000820-CF for qemu-devel@nongnu.org; Thu, 15 Oct 2009 13:49:16 -0400 Received: from fg-out-1718.google.com ([72.14.220.158]:27139) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MyURv-0007je-Um for qemu-devel@nongnu.org; Thu, 15 Oct 2009 13:49:16 -0400 Received: by fg-out-1718.google.com with SMTP id 22so1637731fge.10 for ; Thu, 15 Oct 2009 10:49:14 -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: Thu, 15 Oct 2009 19:49:14 +0200 Message-ID: <761ea48b0910151049l1aa553afn4a29e14d72c1b7ca@mail.gmail.com> Subject: Re: [Qemu-devel] [PATCH 00/18] target-arm cleanup From: Laurent Desnogues 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 , Filip Navara Cc: qemu-devel , Paul Brook On Thu, Oct 15, 2009 at 7:40 PM, Aurelien Jarno wrot= e: > On Sun, Jul 19, 2009 at 05:43:03PM +0200, Filip Navara wrote: >> Hi, > Hi, > >> the set of patches I just sent are intended to improve the TCG >> register allocation in target-arm code. The goals are: >> >> - Use TCG memory-backed variables for the CPU registers >> - Get rid of cpu_T variables >> - Remove unused code >> >> Several emulation correctness bugs are also fixed by the patches, but >> I couldn't verify if the fixes actually work (VUZP and RFE >> instructions). >> > > I have just reviewed this series of patches, you've done a good job! I > have only a few minor comments. > > 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. > > 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. > 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. > > 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. > > Otherwise, I have no specific comments to the individual patches. > > 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 had promised to take a look at the patches weeks ago, sorry for the delay. But talk about coincidence, I looked at them yesterday :-) I fully agree with Aur=E9lien on everything he wrote (especially the functions that implicitly allocate and free temps, that makes the code hard to follow and potentially fragile). I would go one step further by saying that the temps allocated when entering disas_insn should be allocated on demand where needed. The impact on the speed should be measured, but I think it would be cleaner. And one last thing: enable TCG_DEBUG and fix the two problems that remain ;-) Laurent