Re: [Qemu-devel] [RFC PATCH v1 22/22] target/i386: reimplement (V)P(EQ, CMP)(B, W, D)

[Aleksandar Markovic]

On Wed, Jul 31, 2019 at 9:51 PM Richard Henderson <
address@hidden> wrote:

> On 7/31/19 10:57 AM, Jan Bobek wrote:
> > +static inline void gen_gvec_cmpeq(unsigned vece, uint32_t dofs,
> > +                                  uint32_t aofs, uint32_t bofs,
> > +                                  uint32_t oprsz, uint32_t maxsz)
> > +{
> > +    tcg_gen_gvec_cmp(TCG_COND_EQ, vece, dofs, aofs, bofs, oprsz, maxsz);
> > +}
> ...
> > +static inline void gen_gvec_cmpgt(unsigned vece, uint32_t dofs,
> > +                                  uint32_t aofs, uint32_t bofs,
> > +                                  uint32_t oprsz, uint32_t maxsz)
> > +{
> > +    tcg_gen_gvec_cmp(TCG_COND_GT, vece, dofs, aofs, bofs, oprsz, maxsz);
> > +}
>
> Drop the inlines.
>
>
Why? The compiler will decide at the end of the day, but at least "inline"
here
says that the code author thinks that inlining is desirable, logical, and
expected
in these cases, which is in turn a valuable information for the code reader.

Yours,
Aleksandar



> > +#define gen_pcmpgt_mm(env, s, modrm, vece)   gen_gvec_ld_modrm_mm
> ((env), (s), (modrm), (vece), gen_gvec_cmpgt, 0112)
> > +#define gen_pcmpgt_xmm(env, s, modrm, vece)  gen_gvec_ld_modrm_xmm
> ((env), (s), (modrm), (vece), gen_gvec_cmpgt, 0112)
> > +#define gen_vpcmpgt_xmm(env, s, modrm, vece)
> gen_gvec_ld_modrm_vxmm((env), (s), (modrm), (vece), gen_gvec_cmpgt, 0123)
> > +#define gen_vpcmpgt_ymm(env, s, modrm, vece)
> gen_gvec_ld_modrm_vymm((env), (s), (modrm), (vece), gen_gvec_cmpgt, 0123)
> ...
> > +    case 0x64 | M_0F:                  gen_pcmpgt_mm(env, s, modrm,
> MO_8); return;
> > +    case 0x64 | M_0F | P_66:           gen_pcmpgt_xmm(env, s, modrm,
> MO_8); return;
> > +    case 0x64 | M_0F | P_66 | VEX_128: gen_vpcmpgt_xmm(env, s, modrm,
> MO_8); return;
> > +    case 0x64 | M_0F | P_66 | VEX_256: gen_vpcmpgt_ymm(env, s, modrm,
> MO_8); return;
>
> Looks like my comments vs PAND apply to all of the subsequent patches as
> well.
>  But everything else looks good.
>
>
> r~
>
>