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

[Peter Maydell]

On 26 December 2013 19:31, Richard Henderson <address@hidden> wrote:
> 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.

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".

> 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 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.

I'm trying to separate out "what are we doing?" ie "do we need to
do no segment add, segment add of specified segment, or segment
add of default segment?" from "how do we do it" ie "emit these
tcg instructions".

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. The whole addseg
business appears to be a combination of poorly documented
optimization and dodgy backdoor for the lea case as above.
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? Why is it not OK to skip the addition
of the base address for CS/DS/ES if it was specified by an
override prefix rather than being the default for the insn?

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));
}

thanks
-- PMM