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~
