From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41662) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VwGeO-0004P3-0d for qemu-devel@nongnu.org; Thu, 26 Dec 2013 14:31:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VwGeF-0004lH-KI for qemu-devel@nongnu.org; Thu, 26 Dec 2013 14:31:19 -0500 Received: from mail-pb0-x22c.google.com ([2607:f8b0:400e:c01::22c]:42837) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VwGeF-0004l2-C5 for qemu-devel@nongnu.org; Thu, 26 Dec 2013 14:31:11 -0500 Received: by mail-pb0-f44.google.com with SMTP id rq2so8533569pbb.3 for ; Thu, 26 Dec 2013 11:31:10 -0800 (PST) Sender: Richard Henderson Message-ID: <52BC83FA.2070704@twiddle.net> Date: Thu, 26 Dec 2013 11:31:06 -0800 From: Richard Henderson MIME-Version: 1.0 References: <1385694047-6116-1-git-send-email-rth@twiddle.net> <1385694047-6116-42-git-send-email-rth@twiddle.net> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 41/60] target-i386: Create gen_lea_v_seg List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers On 12/26/2013 10:38 AM, Peter Maydell wrote: > The old MO_16 code for gen_string_movl* doesn't care > about s->addseg, and always performs an add of a segment. > This new code might stop without doing the addition. The only time s->addseg will be false in 16-bit mode is during translation of LEA. I do need the addseg check there for LEA cleanup, but this change should not affect gen_string_movl. >> + a0 = cpu_A0; > > This is also pretty confusing -- we have a parameter "a0" > which isn't the same thing as cpu_A0, and in this case > statement sometimes cpu_A0 is the result we're calculating > based on a0, and sometimes we don't touch cpu_A0 at > all and rely on following code to set it up, and in this case > we use cpu_A0 as a random temporary and then assign > it to a0... While I can agree that a0 vs cpu_A0 might be a tad confusing, a0 is always the current input address, and cpu_A0 is always the output address. I disagree with the characterization "random temporary". Using the output as a temporary in computing the output is totally sensible, given that our most popular host platform is 2-address. > I also find the handling of "ovr_seg < 0" pretty hard to read > in the new code -- the code for "we don't need to add a > segment" and "we do need to add a segment" is all mixed > up together, half the cases in the switch statement return > early and half fall out to maybe go through the code below, > and the code below is only for the "adding a segment" part. > > I suspect it would be clearer if it was written: > > if (ovr_seg < 0 && > (s->aflag == MO_16 || (s->aflag == MO_32 && s->addseg))) { > ovr_seg = def_seg; > } > if (ovr_seg < 0) { > /* generate code for no segment add */ > } else { > /* generate code for segment add */ > } Really? I honestly find that harder to read, because that first condition is so complicated. Better to have it split out in the swtch statement where we're already doing special things for M_16, M_32, and M_64. r~