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

[Richard Henderson <rth@twiddle.net>]

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~