From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54532) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VwYj9-0006tp-CB for qemu-devel@nongnu.org; Fri, 27 Dec 2013 09:49:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VwYj0-0006jV-My for qemu-devel@nongnu.org; Fri, 27 Dec 2013 09:49:27 -0500 Received: from mail-pb0-x231.google.com ([2607:f8b0:400e:c01::231]:52473) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VwYj0-0006jJ-Fl for qemu-devel@nongnu.org; Fri, 27 Dec 2013 09:49:18 -0500 Received: by mail-pb0-f49.google.com with SMTP id jt11so9263951pbb.22 for ; Fri, 27 Dec 2013 06:49:17 -0800 (PST) Sender: Richard Henderson Message-ID: <52BD9368.7090902@twiddle.net> Date: Fri, 27 Dec 2013 06:49:12 -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> <52BC83FA.2070704@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 01:27 PM, Peter Maydell wrote: >> 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. > > Oh, is that the bit that does: > > val = s->addseg; > s->addseg = 0; > gen_lea_modrm(env, s, modrm); > s->addseg = val; > > ? I think we should get rid of that -- s->addseg should always > mean "we can do the segment-base-is-zero optimization", > it shouldn't be a secret hidden parameter to the gen_lea functions > saying "don't do this addition". Perhaps. I'd rather do that as follow-up, since lea_v_seg isn't the top-level function called for LEA. And if we clean up addseg, we might as well clean up popl_esp_hack as well. How about a comment for now? >> 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. > > This is the kind of thing that in an ideal world the register allocator > would deal with. The tcg/README optimisation suggestions > don't say anything about preferring to use X,X,Y rather than X,Y,Z > ops where possible, and typically allocating and using a special > purpose temp is more readable code. I agree that a real register allocator would handle it. I disagree that a special purpose temp would result in more readable code, since then we'd need to add *more* code to deallocate it after the other arithmetic. > More generally, it's pretty unclear to me why we're handling > "use default segment register for instruction" (ie ovr_seg == -1) > differently for the three cases. Because the handling of segments is fundamentally different in each of the three cpu modes? > Why is it OK to skip the addition of the base address for ES > (in the movl_A0_EDI case) when the comment for addseg says > it only applies to CS/DS/ES? Err... when are we skipping the addition in gen_string_movl_A0_EDI? We pass in R_ES as the segment register to use... > It seems to me that we ought to try to get this code to a > point where it looks more like: > if (ovr_seg < 0) { > ovr_seg = def_seg; > } > emit code to get address; > if (!segment_base_guaranteed_zero(s, ovr_seg)) { > emit code to add base to address; > } > > where segment_base_guaranteed_zero() is a helper > function like: > bool segment_base_guaranteed_zero(s, seg) { > /* Return true if we can guarantee at translate time that > * the base address of the specified segment is zero > * (and thus can skip emitting code to add it) > */ > return (!s->addseg && > (seg == R_CS || seg == R_DS || seg == R_SS)); > } That does look much nicer, I agree. r~