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

[Peter Maydell]

On 29 November 2013 03:00, Richard Henderson <address@hidden> wrote:
> Add forgotten zero-extension in the TARGET_X86_64, !CODE64, ss32 case;
> use this new function to implement gen_string_movl_A0_EDI,
> gen_string_movl_A0_ESI, gen_add_A0_ds_seg.

I'm afraid I can't figure out how this code matches up with the previous
implementation of these functions. For example:

> +/* Compute SEG:REG into A0.  SEG is selected from the override segment
> +   (OVR_SEG) and the default segment (DEF_SEG).  OVR_SEG may be -1 to
> +   indicate no override.  */
> +static void gen_lea_v_seg(DisasContext *s, TCGv a0, int def_seg, int ovr_seg)
>  {
> -    int override;
> +    TCGMemOp aflag = s->aflag;
>
> -    override = s->override;
> -    switch (s->aflag) {
> +    switch (aflag) {
>  #ifdef TARGET_X86_64
>      case MO_64:
> -        if (override >= 0) {
> -            gen_op_movq_A0_seg(override);
> -            gen_op_addq_A0_reg_sN(0, R_ESI);
> -        } else {
> -            gen_op_movq_A0_reg(R_ESI);
> +        if (ovr_seg < 0) {
> +            tcg_gen_mov_tl(cpu_A0, a0);
> +            return;
>          }
>          break;
>  #endif
>      case MO_32:
>          /* 32 bit address */
> -        if (s->addseg && override < 0)
> -            override = R_DS;
> -        if (override >= 0) {
> -            gen_op_movl_A0_seg(override);
> -            gen_op_addl_A0_reg_sN(0, R_ESI);
> -        } else {
> -            gen_op_movl_A0_reg(R_ESI);
> +        if (ovr_seg < 0) {
> +            if (s->addseg) {
> +                ovr_seg = def_seg;
> +            } else {
> +                tcg_gen_ext32u_tl(cpu_A0, a0);
> +                return;
> +            }
>          }
>          break;
>      case MO_16:
> -        /* 16 address, always override */
> -        if (override < 0)
> -            override = R_DS;
> -        tcg_gen_ext16u_tl(cpu_A0, cpu_regs[R_ESI]);
> -        gen_op_addl_A0_seg(s, override);
> +        /* 16 bit address */
> +        if (ovr_seg < 0) {
> +            ovr_seg = def_seg;
> +        }
> +        tcg_gen_ext16u_tl(cpu_A0, a0);
> +        if (!s->addseg) {
> +            return;
> +        }

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.

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

>          break;
>      default:
>          tcg_abort();
>      }
> -}

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 */
   }

thanks
-- PMM