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