Re: [Qemu-devel] [PATCH v2 41/60] target-i386: Create gen_lea_v_seg

[Richard Henderson <rth@twiddle.net>]

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~