[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: |
Aleksandar Markovic |
Subject: |
Re: [Qemu-devel] [RFC PATCH v1 08/22] target/i386: reimplement (V)PAND, (V)ANDPS, (V)ANDPD |
Date: |
Wed, 31 Jul 2019 22:27:17 +0200 |
On Wed, Jul 31, 2019 at 9:36 PM Richard Henderson <
address@hidden> 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~
>
>
- [Qemu-devel] [RFC PATCH v1 05/22] target/i386: introduce gen_ld_modrm_* helpers, (continued)
- [Qemu-devel] [RFC PATCH v1 05/22] target/i386: introduce gen_ld_modrm_* helpers, Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 04/22] target/i386: Simplify gen_exception arguments, Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 07/22] target/i386: add vector register file alignment constraints, Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 09/22] target/i386: reimplement (V)POR, (V)ORPS, (V)ORPD, Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 06/22] target/i386: introduce gen_gvec_ld_modrm_* helpers, Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 08/22] target/i386: reimplement (V)PAND, (V)ANDPS, (V)ANDPD, Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 10/22] target/i386: reimplement (V)PXOR, (V)XORPS, (V)XORPD, Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 03/22] target/i386: Use prefix, aflag and dflag from DisasContext, Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 11/22] target/i386: reimplement (V)PANDN, (V)ANDNPS, (V)ANDNPD, Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 12/22] target/i386: reimplement (V)PADD(B, W, D, Q), Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 13/22] target/i386: reimplement (V)PSUB(B, W, D, Q), Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 14/22] target/i386: reimplement (V)PADDS(B, W), Jan Bobek, 2019/07/31
- [Qemu-devel] [RFC PATCH v1 15/22] target/i386: reimplement (V)PADDUS(B, W), Jan Bobek, 2019/07/31