Re: [Qemu-devel] [RFC PATCH v1 08/22] target/i386: reimplement (V)PAND, (V)ANDPS, (V)ANDPD

[Richard Henderson <richard.henderson@linaro.org>]

On 7/31/19 10:56 AM, Jan Bobek wrote:
> +#define gen_pand_mm(env, s, modrm)   gen_gvec_ld_modrm_mm  ((env), (s), (modrm), MO_64, tcg_gen_gvec_and, 0112)
> +#define gen_pand_xmm(env, s, modrm)  gen_gvec_ld_modrm_xmm ((env), (s), (modrm), MO_64, tcg_gen_gvec_and, 0112)
> +#define gen_vpand_xmm(env, s, modrm) gen_gvec_ld_modrm_vxmm((env), (s), (modrm), MO_64, tcg_gen_gvec_and, 0123)
> +#define gen_vpand_ymm(env, s, modrm) gen_gvec_ld_modrm_vymm((env), (s), (modrm), MO_64, tcg_gen_gvec_and, 0123)
> +#define gen_andps_xmm  gen_pand_xmm
> +#define gen_vandps_xmm gen_vpand_xmm
> +#define gen_vandps_ymm gen_vpand_ymm
> +#define gen_andpd_xmm  gen_pand_xmm
> +#define gen_vandpd_xmm gen_vpand_xmm
> +#define gen_vandpd_ymm gen_vpand_ymm


Why all of these extra defines?

> +    enum {
> +        M_0F    = 0x01 << 8,
> +        M_0F38  = 0x02 << 8,
> +        M_0F3A  = 0x04 << 8,
> +        P_66    = 0x08 << 8,
> +        P_F3    = 0x10 << 8,
> +        P_F2    = 0x20 << 8,
> +        VEX_128 = 0x40 << 8,
> +        VEX_256 = 0x80 << 8,
> +    };
> +
> +    switch(b | M_0F
> +           | (s->prefix & PREFIX_DATA ? P_66 : 0)
> +           | (s->prefix & PREFIX_REPZ ? P_F3 : 0)
> +           | (s->prefix & PREFIX_REPNZ ? P_F2 : 0)
> +           | (s->prefix & PREFIX_VEX ? (s->vex_l ? VEX_256 : VEX_128) : 0)) {

I think you can move this above almost everything in this function, so that all
of the legacy bits follow this switch.

> +    case 0xdb | M_0F:                  gen_pand_mm(env, s, modrm); return;

You'll want to put these on the next lines -- checkpatch.pl again.

> +    case 0xdb | M_0F | P_66:           gen_pand_xmm(env, s, modrm); return;
> +    case 0xdb | M_0F | P_66 | VEX_128: gen_vpand_xmm(env, s, modrm); return;
> +    case 0xdb | M_0F | P_66 | VEX_256: gen_vpand_ymm(env, s, modrm); return;
> +    case 0x54 | M_0F:                  gen_andps_xmm(env, s, modrm); return;
> +    case 0x54 | M_0F | VEX_128:        gen_vandps_xmm(env, s, modrm); return;
> +    case 0x54 | M_0F | VEX_256:        gen_vandps_ymm(env, s, modrm); return;
> +    case 0x54 | M_0F | P_66:           gen_andpd_xmm(env, s, modrm); return;
> +    case 0x54 | M_0F | P_66 | VEX_128: gen_vandpd_xmm(env, s, modrm); return;
> +    case 0x54 | M_0F | P_66 | VEX_256: gen_vandpd_ymm(env, s, modrm); return;
> +    default: break;
> +    }

Perhaps group cases together?

    case 0xdb | M_0F | P_66:  /* PAND */
    case 0x54 | M_0F:         /* ANDPS */
    case 0x54 | M_0F | P_66:  /* ANDPD */
       gen_gvec_ld_modrm_xmm(env, s, modrm, MO_64, tcg_gen_gvec_and, 0112);
       return;

How are you planning to handle CPUID checks?  I know the currently handling is
quite spotty, but with a reorg we might as well fix that too.


r~