From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJJIT-0006Ox-CE for qemu-devel@nongnu.org; Tue, 10 Sep 2013 04:27:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VJJIN-0000zP-Bt for qemu-devel@nongnu.org; Tue, 10 Sep 2013 04:27:41 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:7332) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJJIN-0000qQ-3R for qemu-devel@nongnu.org; Tue, 10 Sep 2013 04:27:35 -0400 Message-ID: <522ED7D9.4000009@huawei.com> Date: Tue, 10 Sep 2013 10:27:05 +0200 From: Claudio Fontana MIME-Version: 1.0 References: <1378144503-15808-1-git-send-email-rth@twiddle.net> <522D8339.1090002@huawei.com> <522DD679.9030201@twiddle.net> <522DE2EF.1010902@huawei.com> <522DE416.80406@twiddle.net> In-Reply-To: <522DE416.80406@twiddle.net> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 00/29] tcg-aarch64 improvements List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: qemu-devel@nongnu.org On 09.09.2013 17:07, Richard Henderson wrote: > On 09/09/2013 08:02 AM, Claudio Fontana wrote: >> On 09.09.2013 16:08, Richard Henderson wrote: >>> On 09/09/2013 01:13 AM, Claudio Fontana wrote: >>>> after carefully reading and testing your patches, this is how I suggest to proceed: >>>> >>>> first do the implementation of the new functionality (tcg opcodes, jit) in a way that is consistent with the existing code. >>>> No type changes, no refactoring, no beautification. >>>> >>>> Once we agree on those, introduce the meaningful restructuring you want to do, >>>> like the new INSN type, the "don't handle mov/movi in tcg_out_op", the TCG_OPF_64BIT thing, etc. >>>> >>>> Last do the cosmetic stuff if you really want to do it, like the change all ext to bool (note that there is no point if the callers still use "1" and "0": adapt them as well) etc. >>> >>> No, I don't agree. Especially with respect to the insn type. >>> >>> I'd much rather do all the "cosmetic stuff", as you put it, first. It makes >>> all of the "real" changes much easier to understand. >>> >>> >>> r~ >>> >> >> I guess we are stuck then. With the cosmetic and restructuring stuff coming before, I cannot cherry pick the good parts later. >> >> > > Have you tested the first 9 patches on their own? I.e. > >> tcg-aarch64: Set ext based on TCG_OPF_64BIT >> tcg-aarch64: Change all ext variables to bool >> tcg-aarch64: Don't handle mov/movi in tcg_out_op >> tcg-aarch64: Hoist common argument loads in tcg_out_op >> tcg-aarch64: Change enum aarch64_arith_opc to AArch64Insn >> tcg-aarch64: Merge enum aarch64_srr_opc with AArch64Insn >> tcg-aarch64: Introduce tcg_fmt_* functions >> tcg-aarch64: Introduce tcg_fmt_Rdn_aimm >> tcg-aarch64: Implement mov with tcg_fmt_* functions yes. > > There should be no functional change to the backend, producing 100% identical > output code. There should even be little or no change in tcg.o itself. There are two aspects. On one side, although some changes do not break anything, I see some problems in them. Putting them as a prerequisite for the rest forces us to agreeing on everything before moving forward, instead of being able to agree on separate chunks (meat first, rest later). In my view, this makes the process longer. On another side, I end up having to manually revert some parts of these which you put as prerequisites, during bisection when landing after them, which is a huge time drain when tracking regressions introduced in the later part of the series. > This should make it trivial to verify that these patches are not at fault. > > r~ They don't break the targets, no. Claudio