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

Use the gvec infrastructure to achieve the desired functionality.

Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 target/i386/ops_sse.h        | 13 -------
 target/i386/ops_sse_header.h |  8 -----
 target/i386/translate.c      | 66 ++++++++++++++++++++++++++++++++----
 3 files changed, 60 insertions(+), 27 deletions(-)

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.

> +#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~

On Wed, Jul 31, 2019 at 9:51 PM Richard Henderson <
richard.henderson@linaro.org> 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~
>
>

On 7/31/19 1:09 PM, Aleksandar Markovic wrote:
> 
> 
> On Wed, Jul 31, 2019 at 9:51 PM Richard Henderson <richard.henderson@linaro.org
> <mailto:richard.henderson@linaro.org>> 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.

In this case it is in fact a lie that will only confuse the reader, as it did
you.  Functions whose address are passed as a callback, as these are, are
always forced out of line.

But beyond that, clang diagnoses unused static inline within *.c while gcc does
not (I'm not sure I agree with clang, but it is what it is).  By leaving off
the inline, but compilers will diagnose when code rearrangement leaves a
function unused.


r~

On 7/31/19 5:31 PM, Richard Henderson wrote:
> On 7/31/19 1:09 PM, Aleksandar Markovic wrote:
>>
>>
>> On Wed, Jul 31, 2019 at 9:51 PM Richard Henderson <richard.henderson@linaro.org
>> <mailto:richard.henderson@linaro.org>> 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.
> 
> In this case it is in fact a lie that will only confuse the reader, as it did
> you.  Functions whose address are passed as a callback, as these are, are
> always forced out of line.
> 
> But beyond that, clang diagnoses unused static inline within *.c while gcc does
> not (I'm not sure I agree with clang, but it is what it is).  By leaving off
> the inline, but compilers will diagnose when code rearrangement leaves a
> function unused.

Dang, I completely forgot about the function-address vs. inlining rule. I thought
of these as macros, really; they are only functions because I needed to pass
them to the gen_gvec_ld_modrm_* helpers.

I'll drop the inline, compilers ignore it anyway.

-Jan

>
>
>   Functions whose address are passed as a callback, as these are, are
> always forced out of line.
>
>
OK, Richard. However, on a much higher level than this single patch, I am
really
curious about this: what would be the rationale beyond the use of callbacks
in TCG
vector support interface? What is, in fact, achieved with such interface
design that
could not be achieved with callback-less approach?

Thanks,
Aleksandar