[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH v1 08/22] target/i386: reimplement (V)PAND,
From: |
Jan Bobek |
Subject: |
Re: [Qemu-devel] [RFC PATCH v1 08/22] target/i386: reimplement (V)PAND, (V)ANDPS, (V)ANDPD |
Date: |
Fri, 2 Aug 2019 09:53:33 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
On 7/31/19 3:35 PM, Richard Henderson wrote:
> 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;
As Aleksandar pointed out in his email, the general intuition was to
have self-documenting code. Seeing
case 0x54 | M_0F | VEX_256: gen_vandps_ymm(env, s, modrm); return;
clearly states that this particular case is a VANDPS, and if one wants
to see what we do with it, they can go look gen_vandps_ymm up.
That being said, I have to the conclusion in the meantime that keeping
all the extra macros is just too much code and not worth it, so I'll
do it like you suggest above.
> 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.
Good question. CPUID checks are not handled in this patch at all, I
will need to come up with a workable approach.
-Jan
signature.asc
Description: OpenPGP digital signature
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [RFC PATCH v1 08/22] target/i386: reimplement (V)PAND, (V)ANDPS, (V)ANDPD,
Jan Bobek <=