On Wed, Jul 31, 2019 at 9:36 PM Richard Henderson < richard.hender...@linaro.org> 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? > Because of code clarity and safety, I would say. This line: case 0x54 | M_0F: gen_andps_xmm(env, s, modrm); return; looks much clearer than this one: case 0x54 | M_0F: gen_gvec_ld_modrm_mm(env, s, modrm, MO_64, tcg_gen_gvec_and, 0112) and such organization is also much less prone to copy/paste bugs etc. Needless to say there is no performance price at all, so it is a no-brainer. Sincerely, Aleksandar > > > + 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~ > >