diff mbox series

[vect] Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.

Message ID 20230602010015.2571612-1-hongtao.liu@intel.com
State New
Headers
Series [vect] Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed. |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed

Commit Message

Liu, Hongtao June 2, 2023, 1 a.m. UTC 
  We have already use intermidate type in case WIDEN, but not for NONE,
this patch extended that.

I didn't do that in pattern recog since we need to know whether the
stmt belongs to any slp_node to decide the vectype, the related optabs
are checked according to vectype_in and vectype_out. For non-slp case,
vec_pack/unpack are always used when lhs has different size from rhs,
for slp case, sometimes vec_pack/unpack is used, somethings
direct conversion is used.

Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
Ok for trunk?

gcc/ChangeLog:

	PR target/110018
	* tree-vect-stmts.cc (vectorizable_conversion): Use
	intermiediate integer type for float_expr/fix_trunc_expr when
	direct optab is not existed.

gcc/testsuite/ChangeLog:

	* gcc.target/i386/pr110018-1.c: New test.
---
 gcc/testsuite/gcc.target/i386/pr110018-1.c | 94 ++++++++++++++++++++++
 gcc/tree-vect-stmts.cc                     | 56 ++++++++++++-
 2 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr110018-1.c

Comments

Richard Biener June 20, 2023, 8:38 a.m. UTC | #1 
On Fri, Jun 2, 2023 at 3:01 AM liuhongt via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> We have already use intermidate type in case WIDEN, but not for NONE,
> this patch extended that.
>
> I didn't do that in pattern recog since we need to know whether the
> stmt belongs to any slp_node to decide the vectype, the related optabs
> are checked according to vectype_in and vectype_out. For non-slp case,
> vec_pack/unpack are always used when lhs has different size from rhs,
> for slp case, sometimes vec_pack/unpack is used, somethings
> direct conversion is used.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
> Ok for trunk?
>
> gcc/ChangeLog:
>
>         PR target/110018
>         * tree-vect-stmts.cc (vectorizable_conversion): Use
>         intermiediate integer type for float_expr/fix_trunc_expr when
>         direct optab is not existed.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/i386/pr110018-1.c: New test.
> ---
>  gcc/testsuite/gcc.target/i386/pr110018-1.c | 94 ++++++++++++++++++++++
>  gcc/tree-vect-stmts.cc                     | 56 ++++++++++++-
>  2 files changed, 149 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr110018-1.c
>
> diff --git a/gcc/testsuite/gcc.target/i386/pr110018-1.c b/gcc/testsuite/gcc.target/i386/pr110018-1.c
> new file mode 100644
> index 00000000000..b1baffd7af1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr110018-1.c
> @@ -0,0 +1,94 @@
> +/* { dg-do compile } */
> +/* { dg-options "-mavx512fp16 -mavx512vl -O2 -mavx512dq" } */
> +/* { dg-final { scan-assembler-times {(?n)vcvttp[dsh]2[dqw]} 5 } } */
> +/* { dg-final { scan-assembler-times {(?n)vcvt[dqw]*2p[dsh]} 5 } } */
> +
> +void
> +foo (double* __restrict a, char* b)
> +{
> +  a[0] = b[0];
> +  a[1] = b[1];
> +}
> +
> +void
> +foo1 (float* __restrict a, char* b)
> +{
> +  a[0] = b[0];
> +  a[1] = b[1];
> +  a[2] = b[2];
> +  a[3] = b[3];
> +}
> +
> +void
> +foo2 (_Float16* __restrict a, char* b)
> +{
> +  a[0] = b[0];
> +  a[1] = b[1];
> +  a[2] = b[2];
> +  a[3] = b[3];
> +  a[4] = b[4];
> +  a[5] = b[5];
> +  a[6] = b[6];
> +  a[7] = b[7];
> +}
> +
> +void
> +foo3 (double* __restrict a, short* b)
> +{
> +  a[0] = b[0];
> +  a[1] = b[1];
> +}
> +
> +void
> +foo4 (float* __restrict a, char* b)
> +{
> +  a[0] = b[0];
> +  a[1] = b[1];
> +  a[2] = b[2];
> +  a[3] = b[3];
> +}
> +
> +void
> +foo5 (double* __restrict b, char* a)
> +{
> +  a[0] = b[0];
> +  a[1] = b[1];
> +}
> +
> +void
> +foo6 (float* __restrict b, char* a)
> +{
> +  a[0] = b[0];
> +  a[1] = b[1];
> +  a[2] = b[2];
> +  a[3] = b[3];
> +}
> +
> +void
> +foo7 (_Float16* __restrict b, char* a)
> +{
> +  a[0] = b[0];
> +  a[1] = b[1];
> +  a[2] = b[2];
> +  a[3] = b[3];
> +  a[4] = b[4];
> +  a[5] = b[5];
> +  a[6] = b[6];
> +  a[7] = b[7];
> +}
> +
> +void
> +foo8 (double* __restrict b, short* a)
> +{
> +  a[0] = b[0];
> +  a[1] = b[1];
> +}
> +
> +void
> +foo9 (float* __restrict b, char* a)
> +{
> +  a[0] = b[0];
> +  a[1] = b[1];
> +  a[2] = b[2];
> +  a[3] = b[3];
> +}
> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
> index bd3b07a3aa1..1118c89686d 100644
> --- a/gcc/tree-vect-stmts.cc
> +++ b/gcc/tree-vect-stmts.cc
> @@ -5162,6 +5162,49 @@ vectorizable_conversion (vec_info *vinfo,
>         return false;
>        if (supportable_convert_operation (code, vectype_out, vectype_in, &code1))
>         break;

A comment would be nice here.  Like

   /* For conversions between float and smaller integer types try whether we can
      use intermediate signed integer types to support the conversion.  */

> +      if ((code == FLOAT_EXPR
> +          && GET_MODE_SIZE (lhs_mode) > GET_MODE_SIZE (rhs_mode))
> +         || (code == FIX_TRUNC_EXPR
> +             && GET_MODE_SIZE (rhs_mode) > GET_MODE_SIZE (lhs_mode)))
> +       {
> +         bool float_expr_p = code == FLOAT_EXPR;
> +         scalar_mode imode = float_expr_p ? rhs_mode : lhs_mode;
> +         fltsz = GET_MODE_SIZE (float_expr_p ? lhs_mode : rhs_mode);
> +         code1 = float_expr_p ? code : NOP_EXPR;
> +         codecvt1 = float_expr_p ? NOP_EXPR : code;
> +         FOR_EACH_2XWIDER_MODE (rhs_mode_iter, imode)
> +           {
> +             imode = rhs_mode_iter.require ();
> +             if (GET_MODE_SIZE (imode) > fltsz)
> +               break;
> +
> +             cvt_type
> +               = build_nonstandard_integer_type (GET_MODE_BITSIZE (imode),
> +                                                 0);
> +             cvt_type = get_vectype_for_scalar_type (vinfo, cvt_type,
> +                                                     slp_node);
> +             /* This should only happened for SLP as long as loop vectorizer
> +                only supports same-sized vector.  */
> +             if (cvt_type == NULL_TREE
> +                 || maybe_ne (TYPE_VECTOR_SUBPARTS (cvt_type), nunits_in)
> +                 || !supportable_convert_operation (code1, vectype_out,
> +                                                    cvt_type, &code1)
> +                 || !supportable_convert_operation (codecvt1, cvt_type,
> +                                                    vectype_in, &codecvt1))
> +               continue;
> +
> +             found_mode = true;
> +             break;
> +           }
> +
> +         if (found_mode)
> +           {
> +             multi_step_cvt++;
> +             interm_types.safe_push (cvt_type);
> +             cvt_type = NULL_TREE;
> +             break;
> +           }
> +       }
>        /* FALLTHRU */
>      unsupported:
>        if (dump_enabled_p ())
> @@ -5381,7 +5424,18 @@ vectorizable_conversion (vec_info *vinfo,
>         {
>           /* Arguments are ready, create the new vector stmt.  */
>           gcc_assert (TREE_CODE_LENGTH (code1) == unary_op);
> -         gassign *new_stmt = gimple_build_assign (vec_dest, code1, vop0);
> +         gassign* new_stmt;
> +         if (multi_step_cvt)
> +           {
> +             gcc_assert (multi_step_cvt == 1);
> +             new_stmt = gimple_build_assign (vec_dest, codecvt1, vop0);
> +             new_temp = make_ssa_name (vec_dest, new_stmt);

I wonder how you get away with using vec_dest both for the final and the
intermediate conversion and not involve interm_types[0]?

Otherwise looks good.

Thanks,
Richard.

> +             gimple_assign_set_lhs (new_stmt, new_temp);
> +             vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
> +             vop0 = new_temp;
> +             vec_dest = vec_dsts[0];
> +           }
> +         new_stmt = gimple_build_assign (vec_dest, code1, vop0);
>           new_temp = make_ssa_name (vec_dest, new_stmt);
>           gimple_assign_set_lhs (new_stmt, new_temp);
>           vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
> --
> 2.39.1.388.g2fc9e9ca3c
>
Hongtao Liu June 20, 2023, 9:09 a.m. UTC | #2 
On Tue, Jun 20, 2023 at 4:41 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Fri, Jun 2, 2023 at 3:01 AM liuhongt via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > We have already use intermidate type in case WIDEN, but not for NONE,
> > this patch extended that.
> >
> > I didn't do that in pattern recog since we need to know whether the
> > stmt belongs to any slp_node to decide the vectype, the related optabs
> > are checked according to vectype_in and vectype_out. For non-slp case,
> > vec_pack/unpack are always used when lhs has different size from rhs,
> > for slp case, sometimes vec_pack/unpack is used, somethings
> > direct conversion is used.
> >
> > Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
> > Ok for trunk?
> >
> > gcc/ChangeLog:
> >
> >         PR target/110018
> >         * tree-vect-stmts.cc (vectorizable_conversion): Use
> >         intermiediate integer type for float_expr/fix_trunc_expr when
> >         direct optab is not existed.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.target/i386/pr110018-1.c: New test.
> > ---
> >  gcc/testsuite/gcc.target/i386/pr110018-1.c | 94 ++++++++++++++++++++++
> >  gcc/tree-vect-stmts.cc                     | 56 ++++++++++++-
> >  2 files changed, 149 insertions(+), 1 deletion(-)
> >  create mode 100644 gcc/testsuite/gcc.target/i386/pr110018-1.c
> >
> > diff --git a/gcc/testsuite/gcc.target/i386/pr110018-1.c b/gcc/testsuite/gcc.target/i386/pr110018-1.c
> > new file mode 100644
> > index 00000000000..b1baffd7af1
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/pr110018-1.c
> > @@ -0,0 +1,94 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-mavx512fp16 -mavx512vl -O2 -mavx512dq" } */
> > +/* { dg-final { scan-assembler-times {(?n)vcvttp[dsh]2[dqw]} 5 } } */
> > +/* { dg-final { scan-assembler-times {(?n)vcvt[dqw]*2p[dsh]} 5 } } */
> > +
> > +void
> > +foo (double* __restrict a, char* b)
> > +{
> > +  a[0] = b[0];
> > +  a[1] = b[1];
> > +}
> > +
> > +void
> > +foo1 (float* __restrict a, char* b)
> > +{
> > +  a[0] = b[0];
> > +  a[1] = b[1];
> > +  a[2] = b[2];
> > +  a[3] = b[3];
> > +}
> > +
> > +void
> > +foo2 (_Float16* __restrict a, char* b)
> > +{
> > +  a[0] = b[0];
> > +  a[1] = b[1];
> > +  a[2] = b[2];
> > +  a[3] = b[3];
> > +  a[4] = b[4];
> > +  a[5] = b[5];
> > +  a[6] = b[6];
> > +  a[7] = b[7];
> > +}
> > +
> > +void
> > +foo3 (double* __restrict a, short* b)
> > +{
> > +  a[0] = b[0];
> > +  a[1] = b[1];
> > +}
> > +
> > +void
> > +foo4 (float* __restrict a, char* b)
> > +{
> > +  a[0] = b[0];
> > +  a[1] = b[1];
> > +  a[2] = b[2];
> > +  a[3] = b[3];
> > +}
> > +
> > +void
> > +foo5 (double* __restrict b, char* a)
> > +{
> > +  a[0] = b[0];
> > +  a[1] = b[1];
> > +}
> > +
> > +void
> > +foo6 (float* __restrict b, char* a)
> > +{
> > +  a[0] = b[0];
> > +  a[1] = b[1];
> > +  a[2] = b[2];
> > +  a[3] = b[3];
> > +}
> > +
> > +void
> > +foo7 (_Float16* __restrict b, char* a)
> > +{
> > +  a[0] = b[0];
> > +  a[1] = b[1];
> > +  a[2] = b[2];
> > +  a[3] = b[3];
> > +  a[4] = b[4];
> > +  a[5] = b[5];
> > +  a[6] = b[6];
> > +  a[7] = b[7];
> > +}
> > +
> > +void
> > +foo8 (double* __restrict b, short* a)
> > +{
> > +  a[0] = b[0];
> > +  a[1] = b[1];
> > +}
> > +
> > +void
> > +foo9 (float* __restrict b, char* a)
> > +{
> > +  a[0] = b[0];
> > +  a[1] = b[1];
> > +  a[2] = b[2];
> > +  a[3] = b[3];
> > +}
> > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
> > index bd3b07a3aa1..1118c89686d 100644
> > --- a/gcc/tree-vect-stmts.cc
> > +++ b/gcc/tree-vect-stmts.cc
> > @@ -5162,6 +5162,49 @@ vectorizable_conversion (vec_info *vinfo,
> >         return false;
> >        if (supportable_convert_operation (code, vectype_out, vectype_in, &code1))
> >         break;
>
> A comment would be nice here.  Like
>
>    /* For conversions between float and smaller integer types try whether we can
>       use intermediate signed integer types to support the conversion.  */
>
> > +      if ((code == FLOAT_EXPR
> > +          && GET_MODE_SIZE (lhs_mode) > GET_MODE_SIZE (rhs_mode))
> > +         || (code == FIX_TRUNC_EXPR
> > +             && GET_MODE_SIZE (rhs_mode) > GET_MODE_SIZE (lhs_mode)))
> > +       {
> > +         bool float_expr_p = code == FLOAT_EXPR;
> > +         scalar_mode imode = float_expr_p ? rhs_mode : lhs_mode;
> > +         fltsz = GET_MODE_SIZE (float_expr_p ? lhs_mode : rhs_mode);
> > +         code1 = float_expr_p ? code : NOP_EXPR;
> > +         codecvt1 = float_expr_p ? NOP_EXPR : code;
> > +         FOR_EACH_2XWIDER_MODE (rhs_mode_iter, imode)
> > +           {
> > +             imode = rhs_mode_iter.require ();
> > +             if (GET_MODE_SIZE (imode) > fltsz)
> > +               break;
> > +
> > +             cvt_type
> > +               = build_nonstandard_integer_type (GET_MODE_BITSIZE (imode),
> > +                                                 0);
> > +             cvt_type = get_vectype_for_scalar_type (vinfo, cvt_type,
> > +                                                     slp_node);
> > +             /* This should only happened for SLP as long as loop vectorizer
> > +                only supports same-sized vector.  */
> > +             if (cvt_type == NULL_TREE
> > +                 || maybe_ne (TYPE_VECTOR_SUBPARTS (cvt_type), nunits_in)
> > +                 || !supportable_convert_operation (code1, vectype_out,
> > +                                                    cvt_type, &code1)
> > +                 || !supportable_convert_operation (codecvt1, cvt_type,
> > +                                                    vectype_in, &codecvt1))
> > +               continue;
> > +
> > +             found_mode = true;
> > +             break;
> > +           }
> > +
> > +         if (found_mode)
> > +           {
> > +             multi_step_cvt++;
> > +             interm_types.safe_push (cvt_type);
> > +             cvt_type = NULL_TREE;
> > +             break;
> > +           }
> > +       }
> >        /* FALLTHRU */
> >      unsupported:
> >        if (dump_enabled_p ())
> > @@ -5381,7 +5424,18 @@ vectorizable_conversion (vec_info *vinfo,
> >         {
> >           /* Arguments are ready, create the new vector stmt.  */
> >           gcc_assert (TREE_CODE_LENGTH (code1) == unary_op);
> > -         gassign *new_stmt = gimple_build_assign (vec_dest, code1, vop0);
> > +         gassign* new_stmt;
> > +         if (multi_step_cvt)
> > +           {
> > +             gcc_assert (multi_step_cvt == 1);
> > +             new_stmt = gimple_build_assign (vec_dest, codecvt1, vop0);
> > +             new_temp = make_ssa_name (vec_dest, new_stmt);
>
> I wonder how you get away with using vec_dest both for the final and the
> intermediate conversion and not involve interm_types[0]?
interm_types[0] is assigned to vect_dest, the original
vect_dest(vectype_out) is pushed to vec_dsts.

  auto_vec<tree> vec_dsts (multi_step_cvt + 1);
  vec_dest = vect_create_destination_var (scalar_dest,
  (cvt_type && modifier == WIDEN)
  ? cvt_type : vectype_out);
  vec_dsts.quick_push (vec_dest);

  if (multi_step_cvt)
    {
      for (i = interm_types.length () - 1;
   interm_types.iterate (i, &intermediate_type); i--)
{
  vec_dest = vect_create_destination_var (scalar_dest,
  intermediate_type);
  vec_dsts.quick_push (vec_dest);
}
    }

>
> Otherwise looks good.
Thanks for review.
>
> Thanks,
> Richard.
>
> > +             gimple_assign_set_lhs (new_stmt, new_temp);
> > +             vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
> > +             vop0 = new_temp;
> > +             vec_dest = vec_dsts[0];
And it's assigned back to vec_dest here.
> > +           }
> > +         new_stmt = gimple_build_assign (vec_dest, code1, vop0);
> >           new_temp = make_ssa_name (vec_dest, new_stmt);
> >           gimple_assign_set_lhs (new_stmt, new_temp);
> >           vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
> > --
> > 2.39.1.388.g2fc9e9ca3c
> >
Richard Biener June 20, 2023, 9:22 a.m. UTC | #3 
On Tue, Jun 20, 2023 at 11:02 AM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Tue, Jun 20, 2023 at 4:41 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Fri, Jun 2, 2023 at 3:01 AM liuhongt via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> > >
> > > We have already use intermidate type in case WIDEN, but not for NONE,
> > > this patch extended that.
> > >
> > > I didn't do that in pattern recog since we need to know whether the
> > > stmt belongs to any slp_node to decide the vectype, the related optabs
> > > are checked according to vectype_in and vectype_out. For non-slp case,
> > > vec_pack/unpack are always used when lhs has different size from rhs,
> > > for slp case, sometimes vec_pack/unpack is used, somethings
> > > direct conversion is used.
> > >
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
> > > Ok for trunk?
> > >
> > > gcc/ChangeLog:
> > >
> > >         PR target/110018
> > >         * tree-vect-stmts.cc (vectorizable_conversion): Use
> > >         intermiediate integer type for float_expr/fix_trunc_expr when
> > >         direct optab is not existed.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >         * gcc.target/i386/pr110018-1.c: New test.
> > > ---
> > >  gcc/testsuite/gcc.target/i386/pr110018-1.c | 94 ++++++++++++++++++++++
> > >  gcc/tree-vect-stmts.cc                     | 56 ++++++++++++-
> > >  2 files changed, 149 insertions(+), 1 deletion(-)
> > >  create mode 100644 gcc/testsuite/gcc.target/i386/pr110018-1.c
> > >
> > > diff --git a/gcc/testsuite/gcc.target/i386/pr110018-1.c b/gcc/testsuite/gcc.target/i386/pr110018-1.c
> > > new file mode 100644
> > > index 00000000000..b1baffd7af1
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/i386/pr110018-1.c
> > > @@ -0,0 +1,94 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-mavx512fp16 -mavx512vl -O2 -mavx512dq" } */
> > > +/* { dg-final { scan-assembler-times {(?n)vcvttp[dsh]2[dqw]} 5 } } */
> > > +/* { dg-final { scan-assembler-times {(?n)vcvt[dqw]*2p[dsh]} 5 } } */
> > > +
> > > +void
> > > +foo (double* __restrict a, char* b)
> > > +{
> > > +  a[0] = b[0];
> > > +  a[1] = b[1];
> > > +}
> > > +
> > > +void
> > > +foo1 (float* __restrict a, char* b)
> > > +{
> > > +  a[0] = b[0];
> > > +  a[1] = b[1];
> > > +  a[2] = b[2];
> > > +  a[3] = b[3];
> > > +}
> > > +
> > > +void
> > > +foo2 (_Float16* __restrict a, char* b)
> > > +{
> > > +  a[0] = b[0];
> > > +  a[1] = b[1];
> > > +  a[2] = b[2];
> > > +  a[3] = b[3];
> > > +  a[4] = b[4];
> > > +  a[5] = b[5];
> > > +  a[6] = b[6];
> > > +  a[7] = b[7];
> > > +}
> > > +
> > > +void
> > > +foo3 (double* __restrict a, short* b)
> > > +{
> > > +  a[0] = b[0];
> > > +  a[1] = b[1];
> > > +}
> > > +
> > > +void
> > > +foo4 (float* __restrict a, char* b)
> > > +{
> > > +  a[0] = b[0];
> > > +  a[1] = b[1];
> > > +  a[2] = b[2];
> > > +  a[3] = b[3];
> > > +}
> > > +
> > > +void
> > > +foo5 (double* __restrict b, char* a)
> > > +{
> > > +  a[0] = b[0];
> > > +  a[1] = b[1];
> > > +}
> > > +
> > > +void
> > > +foo6 (float* __restrict b, char* a)
> > > +{
> > > +  a[0] = b[0];
> > > +  a[1] = b[1];
> > > +  a[2] = b[2];
> > > +  a[3] = b[3];
> > > +}
> > > +
> > > +void
> > > +foo7 (_Float16* __restrict b, char* a)
> > > +{
> > > +  a[0] = b[0];
> > > +  a[1] = b[1];
> > > +  a[2] = b[2];
> > > +  a[3] = b[3];
> > > +  a[4] = b[4];
> > > +  a[5] = b[5];
> > > +  a[6] = b[6];
> > > +  a[7] = b[7];
> > > +}
> > > +
> > > +void
> > > +foo8 (double* __restrict b, short* a)
> > > +{
> > > +  a[0] = b[0];
> > > +  a[1] = b[1];
> > > +}
> > > +
> > > +void
> > > +foo9 (float* __restrict b, char* a)
> > > +{
> > > +  a[0] = b[0];
> > > +  a[1] = b[1];
> > > +  a[2] = b[2];
> > > +  a[3] = b[3];
> > > +}
> > > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
> > > index bd3b07a3aa1..1118c89686d 100644
> > > --- a/gcc/tree-vect-stmts.cc
> > > +++ b/gcc/tree-vect-stmts.cc
> > > @@ -5162,6 +5162,49 @@ vectorizable_conversion (vec_info *vinfo,
> > >         return false;
> > >        if (supportable_convert_operation (code, vectype_out, vectype_in, &code1))
> > >         break;
> >
> > A comment would be nice here.  Like
> >
> >    /* For conversions between float and smaller integer types try whether we can
> >       use intermediate signed integer types to support the conversion.  */
> >
> > > +      if ((code == FLOAT_EXPR
> > > +          && GET_MODE_SIZE (lhs_mode) > GET_MODE_SIZE (rhs_mode))
> > > +         || (code == FIX_TRUNC_EXPR
> > > +             && GET_MODE_SIZE (rhs_mode) > GET_MODE_SIZE (lhs_mode)))
> > > +       {
> > > +         bool float_expr_p = code == FLOAT_EXPR;
> > > +         scalar_mode imode = float_expr_p ? rhs_mode : lhs_mode;
> > > +         fltsz = GET_MODE_SIZE (float_expr_p ? lhs_mode : rhs_mode);
> > > +         code1 = float_expr_p ? code : NOP_EXPR;
> > > +         codecvt1 = float_expr_p ? NOP_EXPR : code;
> > > +         FOR_EACH_2XWIDER_MODE (rhs_mode_iter, imode)
> > > +           {
> > > +             imode = rhs_mode_iter.require ();
> > > +             if (GET_MODE_SIZE (imode) > fltsz)
> > > +               break;
> > > +
> > > +             cvt_type
> > > +               = build_nonstandard_integer_type (GET_MODE_BITSIZE (imode),
> > > +                                                 0);
> > > +             cvt_type = get_vectype_for_scalar_type (vinfo, cvt_type,
> > > +                                                     slp_node);
> > > +             /* This should only happened for SLP as long as loop vectorizer
> > > +                only supports same-sized vector.  */
> > > +             if (cvt_type == NULL_TREE
> > > +                 || maybe_ne (TYPE_VECTOR_SUBPARTS (cvt_type), nunits_in)
> > > +                 || !supportable_convert_operation (code1, vectype_out,
> > > +                                                    cvt_type, &code1)
> > > +                 || !supportable_convert_operation (codecvt1, cvt_type,
> > > +                                                    vectype_in, &codecvt1))
> > > +               continue;
> > > +
> > > +             found_mode = true;
> > > +             break;
> > > +           }
> > > +
> > > +         if (found_mode)
> > > +           {
> > > +             multi_step_cvt++;
> > > +             interm_types.safe_push (cvt_type);
> > > +             cvt_type = NULL_TREE;
> > > +             break;
> > > +           }
> > > +       }
> > >        /* FALLTHRU */
> > >      unsupported:
> > >        if (dump_enabled_p ())
> > > @@ -5381,7 +5424,18 @@ vectorizable_conversion (vec_info *vinfo,
> > >         {
> > >           /* Arguments are ready, create the new vector stmt.  */
> > >           gcc_assert (TREE_CODE_LENGTH (code1) == unary_op);
> > > -         gassign *new_stmt = gimple_build_assign (vec_dest, code1, vop0);
> > > +         gassign* new_stmt;
> > > +         if (multi_step_cvt)
> > > +           {
> > > +             gcc_assert (multi_step_cvt == 1);
> > > +             new_stmt = gimple_build_assign (vec_dest, codecvt1, vop0);
> > > +             new_temp = make_ssa_name (vec_dest, new_stmt);
> >
> > I wonder how you get away with using vec_dest both for the final and the
> > intermediate conversion and not involve interm_types[0]?
> interm_types[0] is assigned to vect_dest, the original
> vect_dest(vectype_out) is pushed to vec_dsts.
>
>   auto_vec<tree> vec_dsts (multi_step_cvt + 1);
>   vec_dest = vect_create_destination_var (scalar_dest,
>   (cvt_type && modifier == WIDEN)
>   ? cvt_type : vectype_out);
>   vec_dsts.quick_push (vec_dest);
>
>   if (multi_step_cvt)
>     {
>       for (i = interm_types.length () - 1;
>    interm_types.iterate (i, &intermediate_type); i--)
> {
>   vec_dest = vect_create_destination_var (scalar_dest,
>   intermediate_type);
>   vec_dsts.quick_push (vec_dest);
> }
>     }
>
> >
> > Otherwise looks good.
> Thanks for review.
> >
> > Thanks,
> > Richard.
> >
> > > +             gimple_assign_set_lhs (new_stmt, new_temp);
> > > +             vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
> > > +             vop0 = new_temp;
> > > +             vec_dest = vec_dsts[0];
> And it's assigned back to vec_dest here.

Ah, subtle.

So the patch is OK with the added comment.

Thanks,
Richard.

> > > +           }
> > > +         new_stmt = gimple_build_assign (vec_dest, code1, vop0);
> > >           new_temp = make_ssa_name (vec_dest, new_stmt);
> > >           gimple_assign_set_lhs (new_stmt, new_temp);
> > >           vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
> > > --
> > > 2.39.1.388.g2fc9e9ca3c
> > >
>
>
>
> --
> BR,
> Hongtao
Richard Sandiford June 21, 2023, 9:10 a.m. UTC | #4 
Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Fri, Jun 2, 2023 at 3:01 AM liuhongt via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> We have already use intermidate type in case WIDEN, but not for NONE,
>> this patch extended that.
>>
>> I didn't do that in pattern recog since we need to know whether the
>> stmt belongs to any slp_node to decide the vectype, the related optabs
>> are checked according to vectype_in and vectype_out. For non-slp case,
>> vec_pack/unpack are always used when lhs has different size from rhs,
>> for slp case, sometimes vec_pack/unpack is used, somethings
>> direct conversion is used.
>>
>> Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
>> Ok for trunk?
>>
>> gcc/ChangeLog:
>>
>>         PR target/110018
>>         * tree-vect-stmts.cc (vectorizable_conversion): Use
>>         intermiediate integer type for float_expr/fix_trunc_expr when
>>         direct optab is not existed.
>>
>> gcc/testsuite/ChangeLog:
>>
>>         * gcc.target/i386/pr110018-1.c: New test.
>> ---
>>  gcc/testsuite/gcc.target/i386/pr110018-1.c | 94 ++++++++++++++++++++++
>>  gcc/tree-vect-stmts.cc                     | 56 ++++++++++++-
>>  2 files changed, 149 insertions(+), 1 deletion(-)
>>  create mode 100644 gcc/testsuite/gcc.target/i386/pr110018-1.c
>>
>> diff --git a/gcc/testsuite/gcc.target/i386/pr110018-1.c b/gcc/testsuite/gcc.target/i386/pr110018-1.c
>> new file mode 100644
>> index 00000000000..b1baffd7af1
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/i386/pr110018-1.c
>> @@ -0,0 +1,94 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-mavx512fp16 -mavx512vl -O2 -mavx512dq" } */
>> +/* { dg-final { scan-assembler-times {(?n)vcvttp[dsh]2[dqw]} 5 } } */
>> +/* { dg-final { scan-assembler-times {(?n)vcvt[dqw]*2p[dsh]} 5 } } */
>> +
>> +void
>> +foo (double* __restrict a, char* b)
>> +{
>> +  a[0] = b[0];
>> +  a[1] = b[1];
>> +}
>> +
>> +void
>> +foo1 (float* __restrict a, char* b)
>> +{
>> +  a[0] = b[0];
>> +  a[1] = b[1];
>> +  a[2] = b[2];
>> +  a[3] = b[3];
>> +}
>> +
>> +void
>> +foo2 (_Float16* __restrict a, char* b)
>> +{
>> +  a[0] = b[0];
>> +  a[1] = b[1];
>> +  a[2] = b[2];
>> +  a[3] = b[3];
>> +  a[4] = b[4];
>> +  a[5] = b[5];
>> +  a[6] = b[6];
>> +  a[7] = b[7];
>> +}
>> +
>> +void
>> +foo3 (double* __restrict a, short* b)
>> +{
>> +  a[0] = b[0];
>> +  a[1] = b[1];
>> +}
>> +
>> +void
>> +foo4 (float* __restrict a, char* b)
>> +{
>> +  a[0] = b[0];
>> +  a[1] = b[1];
>> +  a[2] = b[2];
>> +  a[3] = b[3];
>> +}
>> +
>> +void
>> +foo5 (double* __restrict b, char* a)
>> +{
>> +  a[0] = b[0];
>> +  a[1] = b[1];
>> +}
>> +
>> +void
>> +foo6 (float* __restrict b, char* a)
>> +{
>> +  a[0] = b[0];
>> +  a[1] = b[1];
>> +  a[2] = b[2];
>> +  a[3] = b[3];
>> +}
>> +
>> +void
>> +foo7 (_Float16* __restrict b, char* a)
>> +{
>> +  a[0] = b[0];
>> +  a[1] = b[1];
>> +  a[2] = b[2];
>> +  a[3] = b[3];
>> +  a[4] = b[4];
>> +  a[5] = b[5];
>> +  a[6] = b[6];
>> +  a[7] = b[7];
>> +}
>> +
>> +void
>> +foo8 (double* __restrict b, short* a)
>> +{
>> +  a[0] = b[0];
>> +  a[1] = b[1];
>> +}
>> +
>> +void
>> +foo9 (float* __restrict b, char* a)
>> +{
>> +  a[0] = b[0];
>> +  a[1] = b[1];
>> +  a[2] = b[2];
>> +  a[3] = b[3];
>> +}
>> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
>> index bd3b07a3aa1..1118c89686d 100644
>> --- a/gcc/tree-vect-stmts.cc
>> +++ b/gcc/tree-vect-stmts.cc
>> @@ -5162,6 +5162,49 @@ vectorizable_conversion (vec_info *vinfo,
>>         return false;
>>        if (supportable_convert_operation (code, vectype_out, vectype_in, &code1))
>>         break;
>
> A comment would be nice here.  Like
>
>    /* For conversions between float and smaller integer types try whether we can
>       use intermediate signed integer types to support the conversion.  */
>
>> +      if ((code == FLOAT_EXPR
>> +          && GET_MODE_SIZE (lhs_mode) > GET_MODE_SIZE (rhs_mode))
>> +         || (code == FIX_TRUNC_EXPR
>> +             && GET_MODE_SIZE (rhs_mode) > GET_MODE_SIZE (lhs_mode)))

Is the FIX_TRUNC_EXPR case safe without some flag?

#include <stdint.h>
int32_t x = (int32_t)0x1.0p32;
int32_t y = (int32_t)(int64_t)0x1.0p32;

sets x to 2147483647 and y to 0.

Thanks,
Richard

>> +       {
>> +         bool float_expr_p = code == FLOAT_EXPR;
>> +         scalar_mode imode = float_expr_p ? rhs_mode : lhs_mode;
>> +         fltsz = GET_MODE_SIZE (float_expr_p ? lhs_mode : rhs_mode);
>> +         code1 = float_expr_p ? code : NOP_EXPR;
>> +         codecvt1 = float_expr_p ? NOP_EXPR : code;
>> +         FOR_EACH_2XWIDER_MODE (rhs_mode_iter, imode)
>> +           {
>> +             imode = rhs_mode_iter.require ();
>> +             if (GET_MODE_SIZE (imode) > fltsz)
>> +               break;
>> +
>> +             cvt_type
>> +               = build_nonstandard_integer_type (GET_MODE_BITSIZE (imode),
>> +                                                 0);
>> +             cvt_type = get_vectype_for_scalar_type (vinfo, cvt_type,
>> +                                                     slp_node);
>> +             /* This should only happened for SLP as long as loop vectorizer
>> +                only supports same-sized vector.  */
>> +             if (cvt_type == NULL_TREE
>> +                 || maybe_ne (TYPE_VECTOR_SUBPARTS (cvt_type), nunits_in)
>> +                 || !supportable_convert_operation (code1, vectype_out,
>> +                                                    cvt_type, &code1)
>> +                 || !supportable_convert_operation (codecvt1, cvt_type,
>> +                                                    vectype_in, &codecvt1))
>> +               continue;
>> +
>> +             found_mode = true;
>> +             break;
>> +           }
>> +
>> +         if (found_mode)
>> +           {
>> +             multi_step_cvt++;
>> +             interm_types.safe_push (cvt_type);
>> +             cvt_type = NULL_TREE;
>> +             break;
>> +           }
>> +       }
>>        /* FALLTHRU */
>>      unsupported:
>>        if (dump_enabled_p ())
>> @@ -5381,7 +5424,18 @@ vectorizable_conversion (vec_info *vinfo,
>>         {
>>           /* Arguments are ready, create the new vector stmt.  */
>>           gcc_assert (TREE_CODE_LENGTH (code1) == unary_op);
>> -         gassign *new_stmt = gimple_build_assign (vec_dest, code1, vop0);
>> +         gassign* new_stmt;
>> +         if (multi_step_cvt)
>> +           {
>> +             gcc_assert (multi_step_cvt == 1);
>> +             new_stmt = gimple_build_assign (vec_dest, codecvt1, vop0);
>> +             new_temp = make_ssa_name (vec_dest, new_stmt);
>
> I wonder how you get away with using vec_dest both for the final and the
> intermediate conversion and not involve interm_types[0]?
>
> Otherwise looks good.
>
> Thanks,
> Richard.
>
>> +             gimple_assign_set_lhs (new_stmt, new_temp);
>> +             vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
>> +             vop0 = new_temp;
>> +             vec_dest = vec_dsts[0];
>> +           }
>> +         new_stmt = gimple_build_assign (vec_dest, code1, vop0);
>>           new_temp = make_ssa_name (vec_dest, new_stmt);
>>           gimple_assign_set_lhs (new_stmt, new_temp);
>>           vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
>> --
>> 2.39.1.388.g2fc9e9ca3c
>>
Richard Sandiford June 21, 2023, 9:32 a.m. UTC | #5 
Richard Sandiford <richard.sandiford@arm.com> writes:
> Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> On Fri, Jun 2, 2023 at 3:01 AM liuhongt via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>> We have already use intermidate type in case WIDEN, but not for NONE,
>>> this patch extended that.
>>>
>>> I didn't do that in pattern recog since we need to know whether the
>>> stmt belongs to any slp_node to decide the vectype, the related optabs
>>> are checked according to vectype_in and vectype_out. For non-slp case,
>>> vec_pack/unpack are always used when lhs has different size from rhs,
>>> for slp case, sometimes vec_pack/unpack is used, somethings
>>> direct conversion is used.
>>>
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
>>> Ok for trunk?
>>>
>>> gcc/ChangeLog:
>>>
>>>         PR target/110018
>>>         * tree-vect-stmts.cc (vectorizable_conversion): Use
>>>         intermiediate integer type for float_expr/fix_trunc_expr when
>>>         direct optab is not existed.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>         * gcc.target/i386/pr110018-1.c: New test.
>>> ---
>>>  gcc/testsuite/gcc.target/i386/pr110018-1.c | 94 ++++++++++++++++++++++
>>>  gcc/tree-vect-stmts.cc                     | 56 ++++++++++++-
>>>  2 files changed, 149 insertions(+), 1 deletion(-)
>>>  create mode 100644 gcc/testsuite/gcc.target/i386/pr110018-1.c
>>>
>>> diff --git a/gcc/testsuite/gcc.target/i386/pr110018-1.c b/gcc/testsuite/gcc.target/i386/pr110018-1.c
>>> new file mode 100644
>>> index 00000000000..b1baffd7af1
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/i386/pr110018-1.c
>>> @@ -0,0 +1,94 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-mavx512fp16 -mavx512vl -O2 -mavx512dq" } */
>>> +/* { dg-final { scan-assembler-times {(?n)vcvttp[dsh]2[dqw]} 5 } } */
>>> +/* { dg-final { scan-assembler-times {(?n)vcvt[dqw]*2p[dsh]} 5 } } */
>>> +
>>> +void
>>> +foo (double* __restrict a, char* b)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +}
>>> +
>>> +void
>>> +foo1 (float* __restrict a, char* b)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +}
>>> +
>>> +void
>>> +foo2 (_Float16* __restrict a, char* b)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +  a[4] = b[4];
>>> +  a[5] = b[5];
>>> +  a[6] = b[6];
>>> +  a[7] = b[7];
>>> +}
>>> +
>>> +void
>>> +foo3 (double* __restrict a, short* b)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +}
>>> +
>>> +void
>>> +foo4 (float* __restrict a, char* b)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +}
>>> +
>>> +void
>>> +foo5 (double* __restrict b, char* a)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +}
>>> +
>>> +void
>>> +foo6 (float* __restrict b, char* a)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +}
>>> +
>>> +void
>>> +foo7 (_Float16* __restrict b, char* a)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +  a[4] = b[4];
>>> +  a[5] = b[5];
>>> +  a[6] = b[6];
>>> +  a[7] = b[7];
>>> +}
>>> +
>>> +void
>>> +foo8 (double* __restrict b, short* a)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +}
>>> +
>>> +void
>>> +foo9 (float* __restrict b, char* a)
>>> +{
>>> +  a[0] = b[0];
>>> +  a[1] = b[1];
>>> +  a[2] = b[2];
>>> +  a[3] = b[3];
>>> +}
>>> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
>>> index bd3b07a3aa1..1118c89686d 100644
>>> --- a/gcc/tree-vect-stmts.cc
>>> +++ b/gcc/tree-vect-stmts.cc
>>> @@ -5162,6 +5162,49 @@ vectorizable_conversion (vec_info *vinfo,
>>>         return false;
>>>        if (supportable_convert_operation (code, vectype_out, vectype_in, &code1))
>>>         break;
>>
>> A comment would be nice here.  Like
>>
>>    /* For conversions between float and smaller integer types try whether we can
>>       use intermediate signed integer types to support the conversion.  */
>>
>>> +      if ((code == FLOAT_EXPR
>>> +          && GET_MODE_SIZE (lhs_mode) > GET_MODE_SIZE (rhs_mode))
>>> +         || (code == FIX_TRUNC_EXPR
>>> +             && GET_MODE_SIZE (rhs_mode) > GET_MODE_SIZE (lhs_mode)))
>
> Is the FIX_TRUNC_EXPR case safe without some flag?
>
> #include <stdint.h>
> int32_t x = (int32_t)0x1.0p32;
> int32_t y = (int32_t)(int64_t)0x1.0p32;
>
> sets x to 2147483647 and y to 0.

Also, I think multi_step_cvt should influence the costs, since at
the moment we cost one statement but generate two.  This makes a
difference for SVE with VECT_COMPARE_COSTS.  Would changing it to:

	  vect_model_simple_cost (vinfo, stmt_info,
				  ncopies * (multi_step_cvt + 1),
				  dt, ndts, slp_node,
				  cost_vec);

be OK?

There again, I wonder if we should handle this using patterns instead.
That makes both conversions explicit and therefore easier to cost.

E.g. for SVE, an integer extension is free if the source is a load,
and we do try to model that.  But it's difficult to handle if the
conversion is only implicit.

Thanks,
Richard
Richard Biener June 21, 2023, 11:04 a.m. UTC | #6 
On Wed, Jun 21, 2023 at 11:32 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Richard Sandiford <richard.sandiford@arm.com> writes:
> > Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> >> On Fri, Jun 2, 2023 at 3:01 AM liuhongt via Gcc-patches
> >> <gcc-patches@gcc.gnu.org> wrote:
> >>>
> >>> We have already use intermidate type in case WIDEN, but not for NONE,
> >>> this patch extended that.
> >>>
> >>> I didn't do that in pattern recog since we need to know whether the
> >>> stmt belongs to any slp_node to decide the vectype, the related optabs
> >>> are checked according to vectype_in and vectype_out. For non-slp case,
> >>> vec_pack/unpack are always used when lhs has different size from rhs,
> >>> for slp case, sometimes vec_pack/unpack is used, somethings
> >>> direct conversion is used.
> >>>
> >>> Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
> >>> Ok for trunk?
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>>         PR target/110018
> >>>         * tree-vect-stmts.cc (vectorizable_conversion): Use
> >>>         intermiediate integer type for float_expr/fix_trunc_expr when
> >>>         direct optab is not existed.
> >>>
> >>> gcc/testsuite/ChangeLog:
> >>>
> >>>         * gcc.target/i386/pr110018-1.c: New test.
> >>> ---
> >>>  gcc/testsuite/gcc.target/i386/pr110018-1.c | 94 ++++++++++++++++++++++
> >>>  gcc/tree-vect-stmts.cc                     | 56 ++++++++++++-
> >>>  2 files changed, 149 insertions(+), 1 deletion(-)
> >>>  create mode 100644 gcc/testsuite/gcc.target/i386/pr110018-1.c
> >>>
> >>> diff --git a/gcc/testsuite/gcc.target/i386/pr110018-1.c b/gcc/testsuite/gcc.target/i386/pr110018-1.c
> >>> new file mode 100644
> >>> index 00000000000..b1baffd7af1
> >>> --- /dev/null
> >>> +++ b/gcc/testsuite/gcc.target/i386/pr110018-1.c
> >>> @@ -0,0 +1,94 @@
> >>> +/* { dg-do compile } */
> >>> +/* { dg-options "-mavx512fp16 -mavx512vl -O2 -mavx512dq" } */
> >>> +/* { dg-final { scan-assembler-times {(?n)vcvttp[dsh]2[dqw]} 5 } } */
> >>> +/* { dg-final { scan-assembler-times {(?n)vcvt[dqw]*2p[dsh]} 5 } } */
> >>> +
> >>> +void
> >>> +foo (double* __restrict a, char* b)
> >>> +{
> >>> +  a[0] = b[0];
> >>> +  a[1] = b[1];
> >>> +}
> >>> +
> >>> +void
> >>> +foo1 (float* __restrict a, char* b)
> >>> +{
> >>> +  a[0] = b[0];
> >>> +  a[1] = b[1];
> >>> +  a[2] = b[2];
> >>> +  a[3] = b[3];
> >>> +}
> >>> +
> >>> +void
> >>> +foo2 (_Float16* __restrict a, char* b)
> >>> +{
> >>> +  a[0] = b[0];
> >>> +  a[1] = b[1];
> >>> +  a[2] = b[2];
> >>> +  a[3] = b[3];
> >>> +  a[4] = b[4];
> >>> +  a[5] = b[5];
> >>> +  a[6] = b[6];
> >>> +  a[7] = b[7];
> >>> +}
> >>> +
> >>> +void
> >>> +foo3 (double* __restrict a, short* b)
> >>> +{
> >>> +  a[0] = b[0];
> >>> +  a[1] = b[1];
> >>> +}
> >>> +
> >>> +void
> >>> +foo4 (float* __restrict a, char* b)
> >>> +{
> >>> +  a[0] = b[0];
> >>> +  a[1] = b[1];
> >>> +  a[2] = b[2];
> >>> +  a[3] = b[3];
> >>> +}
> >>> +
> >>> +void
> >>> +foo5 (double* __restrict b, char* a)
> >>> +{
> >>> +  a[0] = b[0];
> >>> +  a[1] = b[1];
> >>> +}
> >>> +
> >>> +void
> >>> +foo6 (float* __restrict b, char* a)
> >>> +{
> >>> +  a[0] = b[0];
> >>> +  a[1] = b[1];
> >>> +  a[2] = b[2];
> >>> +  a[3] = b[3];
> >>> +}
> >>> +
> >>> +void
> >>> +foo7 (_Float16* __restrict b, char* a)
> >>> +{
> >>> +  a[0] = b[0];
> >>> +  a[1] = b[1];
> >>> +  a[2] = b[2];
> >>> +  a[3] = b[3];
> >>> +  a[4] = b[4];
> >>> +  a[5] = b[5];
> >>> +  a[6] = b[6];
> >>> +  a[7] = b[7];
> >>> +}
> >>> +
> >>> +void
> >>> +foo8 (double* __restrict b, short* a)
> >>> +{
> >>> +  a[0] = b[0];
> >>> +  a[1] = b[1];
> >>> +}
> >>> +
> >>> +void
> >>> +foo9 (float* __restrict b, char* a)
> >>> +{
> >>> +  a[0] = b[0];
> >>> +  a[1] = b[1];
> >>> +  a[2] = b[2];
> >>> +  a[3] = b[3];
> >>> +}
> >>> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
> >>> index bd3b07a3aa1..1118c89686d 100644
> >>> --- a/gcc/tree-vect-stmts.cc
> >>> +++ b/gcc/tree-vect-stmts.cc
> >>> @@ -5162,6 +5162,49 @@ vectorizable_conversion (vec_info *vinfo,
> >>>         return false;
> >>>        if (supportable_convert_operation (code, vectype_out, vectype_in, &code1))
> >>>         break;
> >>
> >> A comment would be nice here.  Like
> >>
> >>    /* For conversions between float and smaller integer types try whether we can
> >>       use intermediate signed integer types to support the conversion.  */
> >>
> >>> +      if ((code == FLOAT_EXPR
> >>> +          && GET_MODE_SIZE (lhs_mode) > GET_MODE_SIZE (rhs_mode))
> >>> +         || (code == FIX_TRUNC_EXPR
> >>> +             && GET_MODE_SIZE (rhs_mode) > GET_MODE_SIZE (lhs_mode)))
> >
> > Is the FIX_TRUNC_EXPR case safe without some flag?
> >
> > #include <stdint.h>
> > int32_t x = (int32_t)0x1.0p32;
> > int32_t y = (int32_t)(int64_t)0x1.0p32;
> >
> > sets x to 2147483647 and y to 0.

Hmm, good question.  GENERIC has a direct truncation to unsigned char
for example, the C standard generally says if the integral part cannot
be represented then the behavior is undefined.  So I think we should be
safe here (0x1.0p32 doesn't fit an int).

> Also, I think multi_step_cvt should influence the costs, since at
> the moment we cost one statement but generate two.  This makes a
> difference for SVE with VECT_COMPARE_COSTS.  Would changing it to:
>
>           vect_model_simple_cost (vinfo, stmt_info,
>                                   ncopies * (multi_step_cvt + 1),
>                                   dt, ndts, slp_node,
>                                   cost_vec);
>
> be OK?

Yeah, I guess so.

> There again, I wonder if we should handle this using patterns instead.
> That makes both conversions explicit and therefore easier to cost.

But we don't do this for the other multi-step conversions either ...
but sure, I also suggested that but the complaint was that with
BB SLP this would get us a vector type with off lanes?

> E.g. for SVE, an integer extension is free if the source is a load,
> and we do try to model that.  But it's difficult to handle if the
> conversion is only implicit.

In the distant future I hope that vectorizable_conversion will
upon analysis produce an SLP sub-graph for the suggested
code generation which we'd then cost.  My current idea is
to get this part live before switching the analysis to all-SLP
because it "seems" easier (fingers crossing).  Still I'm
struggling to even find time to start that effort.

Richard.

>
> Thanks,
> Richard
Richard Sandiford June 21, 2023, 11:20 a.m. UTC | #7 
Richard Biener <richard.guenther@gmail.com> writes:
> On Wed, Jun 21, 2023 at 11:32 AM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Richard Sandiford <richard.sandiford@arm.com> writes:
>> > Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> >> On Fri, Jun 2, 2023 at 3:01 AM liuhongt via Gcc-patches
>> >> <gcc-patches@gcc.gnu.org> wrote:
>> >>>
>> >>> We have already use intermidate type in case WIDEN, but not for NONE,
>> >>> this patch extended that.
>> >>>
>> >>> I didn't do that in pattern recog since we need to know whether the
>> >>> stmt belongs to any slp_node to decide the vectype, the related optabs
>> >>> are checked according to vectype_in and vectype_out. For non-slp case,
>> >>> vec_pack/unpack are always used when lhs has different size from rhs,
>> >>> for slp case, sometimes vec_pack/unpack is used, somethings
>> >>> direct conversion is used.
>> >>>
>> >>> Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}.
>> >>> Ok for trunk?
>> >>>
>> >>> gcc/ChangeLog:
>> >>>
>> >>>         PR target/110018
>> >>>         * tree-vect-stmts.cc (vectorizable_conversion): Use
>> >>>         intermiediate integer type for float_expr/fix_trunc_expr when
>> >>>         direct optab is not existed.
>> >>>
>> >>> gcc/testsuite/ChangeLog:
>> >>>
>> >>>         * gcc.target/i386/pr110018-1.c: New test.
>> >>> ---
>> >>>  gcc/testsuite/gcc.target/i386/pr110018-1.c | 94 ++++++++++++++++++++++
>> >>>  gcc/tree-vect-stmts.cc                     | 56 ++++++++++++-
>> >>>  2 files changed, 149 insertions(+), 1 deletion(-)
>> >>>  create mode 100644 gcc/testsuite/gcc.target/i386/pr110018-1.c
>> >>>
>> >>> diff --git a/gcc/testsuite/gcc.target/i386/pr110018-1.c b/gcc/testsuite/gcc.target/i386/pr110018-1.c
>> >>> new file mode 100644
>> >>> index 00000000000..b1baffd7af1
>> >>> --- /dev/null
>> >>> +++ b/gcc/testsuite/gcc.target/i386/pr110018-1.c
>> >>> @@ -0,0 +1,94 @@
>> >>> +/* { dg-do compile } */
>> >>> +/* { dg-options "-mavx512fp16 -mavx512vl -O2 -mavx512dq" } */
>> >>> +/* { dg-final { scan-assembler-times {(?n)vcvttp[dsh]2[dqw]} 5 } } */
>> >>> +/* { dg-final { scan-assembler-times {(?n)vcvt[dqw]*2p[dsh]} 5 } } */
>> >>> +
>> >>> +void
>> >>> +foo (double* __restrict a, char* b)
>> >>> +{
>> >>> +  a[0] = b[0];
>> >>> +  a[1] = b[1];
>> >>> +}
>> >>> +
>> >>> +void
>> >>> +foo1 (float* __restrict a, char* b)
>> >>> +{
>> >>> +  a[0] = b[0];
>> >>> +  a[1] = b[1];
>> >>> +  a[2] = b[2];
>> >>> +  a[3] = b[3];
>> >>> +}
>> >>> +
>> >>> +void
>> >>> +foo2 (_Float16* __restrict a, char* b)
>> >>> +{
>> >>> +  a[0] = b[0];
>> >>> +  a[1] = b[1];
>> >>> +  a[2] = b[2];
>> >>> +  a[3] = b[3];
>> >>> +  a[4] = b[4];
>> >>> +  a[5] = b[5];
>> >>> +  a[6] = b[6];
>> >>> +  a[7] = b[7];
>> >>> +}
>> >>> +
>> >>> +void
>> >>> +foo3 (double* __restrict a, short* b)
>> >>> +{
>> >>> +  a[0] = b[0];
>> >>> +  a[1] = b[1];
>> >>> +}
>> >>> +
>> >>> +void
>> >>> +foo4 (float* __restrict a, char* b)
>> >>> +{
>> >>> +  a[0] = b[0];
>> >>> +  a[1] = b[1];
>> >>> +  a[2] = b[2];
>> >>> +  a[3] = b[3];
>> >>> +}
>> >>> +
>> >>> +void
>> >>> +foo5 (double* __restrict b, char* a)
>> >>> +{
>> >>> +  a[0] = b[0];
>> >>> +  a[1] = b[1];
>> >>> +}
>> >>> +
>> >>> +void
>> >>> +foo6 (float* __restrict b, char* a)
>> >>> +{
>> >>> +  a[0] = b[0];
>> >>> +  a[1] = b[1];
>> >>> +  a[2] = b[2];
>> >>> +  a[3] = b[3];
>> >>> +}
>> >>> +
>> >>> +void
>> >>> +foo7 (_Float16* __restrict b, char* a)
>> >>> +{
>> >>> +  a[0] = b[0];
>> >>> +  a[1] = b[1];
>> >>> +  a[2] = b[2];
>> >>> +  a[3] = b[3];
>> >>> +  a[4] = b[4];
>> >>> +  a[5] = b[5];
>> >>> +  a[6] = b[6];
>> >>> +  a[7] = b[7];
>> >>> +}
>> >>> +
>> >>> +void
>> >>> +foo8 (double* __restrict b, short* a)
>> >>> +{
>> >>> +  a[0] = b[0];
>> >>> +  a[1] = b[1];
>> >>> +}
>> >>> +
>> >>> +void
>> >>> +foo9 (float* __restrict b, char* a)
>> >>> +{
>> >>> +  a[0] = b[0];
>> >>> +  a[1] = b[1];
>> >>> +  a[2] = b[2];
>> >>> +  a[3] = b[3];
>> >>> +}
>> >>> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
>> >>> index bd3b07a3aa1..1118c89686d 100644
>> >>> --- a/gcc/tree-vect-stmts.cc
>> >>> +++ b/gcc/tree-vect-stmts.cc
>> >>> @@ -5162,6 +5162,49 @@ vectorizable_conversion (vec_info *vinfo,
>> >>>         return false;
>> >>>        if (supportable_convert_operation (code, vectype_out, vectype_in, &code1))
>> >>>         break;
>> >>
>> >> A comment would be nice here.  Like
>> >>
>> >>    /* For conversions between float and smaller integer types try whether we can
>> >>       use intermediate signed integer types to support the conversion.  */
>> >>
>> >>> +      if ((code == FLOAT_EXPR
>> >>> +          && GET_MODE_SIZE (lhs_mode) > GET_MODE_SIZE (rhs_mode))
>> >>> +         || (code == FIX_TRUNC_EXPR
>> >>> +             && GET_MODE_SIZE (rhs_mode) > GET_MODE_SIZE (lhs_mode)))
>> >
>> > Is the FIX_TRUNC_EXPR case safe without some flag?
>> >
>> > #include <stdint.h>
>> > int32_t x = (int32_t)0x1.0p32;
>> > int32_t y = (int32_t)(int64_t)0x1.0p32;
>> >
>> > sets x to 2147483647 and y to 0.
>
> Hmm, good question.  GENERIC has a direct truncation to unsigned char
> for example, the C standard generally says if the integral part cannot
> be represented then the behavior is undefined.  So I think we should be
> safe here (0x1.0p32 doesn't fit an int).

OK.

>> Also, I think multi_step_cvt should influence the costs, since at
>> the moment we cost one statement but generate two.  This makes a
>> difference for SVE with VECT_COMPARE_COSTS.  Would changing it to:
>>
>>           vect_model_simple_cost (vinfo, stmt_info,
>>                                   ncopies * (multi_step_cvt + 1),
>>                                   dt, ndts, slp_node,
>>                                   cost_vec);
>>
>> be OK?
>
> Yeah, I guess so.

Thanks, will send a patch.

>> There again, I wonder if we should handle this using patterns instead.
>> That makes both conversions explicit and therefore easier to cost.
>
> But we don't do this for the other multi-step conversions either ...

Yeah, agree it's not a new problem.

> but sure, I also suggested that but the complaint was that with
> BB SLP this would get us a vector type with off lanes?

Ah, sorry, missed the previous discussion.  I only became interested
once some SVE tests started failing :)

>> E.g. for SVE, an integer extension is free if the source is a load,
>> and we do try to model that.  But it's difficult to handle if the
>> conversion is only implicit.
>
> In the distant future I hope that vectorizable_conversion will
> upon analysis produce an SLP sub-graph for the suggested
> code generation which we'd then cost.

Sounds good.  Agree we can live with the compound operation
until then.

> My current idea is
> to get this part live before switching the analysis to all-SLP
> because it "seems" easier (fingers crossing).  Still I'm
> struggling to even find time to start that effort.

Yeah, can sympthaise :/  Don't know how you find time for all
the reviews you currently do.

Thanks,
Richard
Joseph Myers June 21, 2023, 8:39 p.m. UTC | #8 
On Wed, 21 Jun 2023, Richard Biener via Gcc-patches wrote:

> > > int32_t x = (int32_t)0x1.0p32;
> > > int32_t y = (int32_t)(int64_t)0x1.0p32;
> > >
> > > sets x to 2147483647 and y to 0.
> 
> Hmm, good question.  GENERIC has a direct truncation to unsigned char
> for example, the C standard generally says if the integral part cannot
> be represented then the behavior is undefined.  So I think we should be
> safe here (0x1.0p32 doesn't fit an int).

We should be following Annex F (unspecified value plus "invalid" exception 
for out-of-range floating-to-integer conversions rather than undefined 
behavior).  But we don't achieve that very well at present (see bug 93806 
comments 27-29 for examples of how such conversions produce wobbly 
values).
Richard Biener June 22, 2023, 7:32 a.m. UTC | #9 
On Wed, Jun 21, 2023 at 10:39 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Wed, 21 Jun 2023, Richard Biener via Gcc-patches wrote:
>
> > > > int32_t x = (int32_t)0x1.0p32;
> > > > int32_t y = (int32_t)(int64_t)0x1.0p32;
> > > >
> > > > sets x to 2147483647 and y to 0.
> >
> > Hmm, good question.  GENERIC has a direct truncation to unsigned char
> > for example, the C standard generally says if the integral part cannot
> > be represented then the behavior is undefined.  So I think we should be
> > safe here (0x1.0p32 doesn't fit an int).
>
> We should be following Annex F (unspecified value plus "invalid" exception
> for out-of-range floating-to-integer conversions rather than undefined
> behavior).  But we don't achieve that very well at present (see bug 93806
> comments 27-29 for examples of how such conversions produce wobbly
> values).

That would mean guarding this with !flag_trapping_math would be the appropriate
thing to do.

Richard.

> --
> Joseph S. Myers
> joseph@codesourcery.com
Robin Dapp July 12, 2023, 9:19 a.m. UTC | #10 
>>>>> int32_t x = (int32_t)0x1.0p32;
>>>>> int32_t y = (int32_t)(int64_t)0x1.0p32;
>>>>>
>>>>> sets x to 2147483647 and y to 0.
>>>
>>> Hmm, good question.  GENERIC has a direct truncation to unsigned char
>>> for example, the C standard generally says if the integral part cannot
>>> be represented then the behavior is undefined.  So I think we should be
>>> safe here (0x1.0p32 doesn't fit an int).
>>
>> We should be following Annex F (unspecified value plus "invalid" exception
>> for out-of-range floating-to-integer conversions rather than undefined
>> behavior).  But we don't achieve that very well at present (see bug 93806
>> comments 27-29 for examples of how such conversions produce wobbly
>> values).
> 
> That would mean guarding this with !flag_trapping_math would be the appropriate
> thing to do.

Follow-up on this:  When we do a NARROW_DST multiple-step conversion we
do not guard with !flag_trapping_math.  Is this intentional and if so, why
do we only require it for the NONE case?

I was thinking of implementing an expander for double -> int16 conversion
for RISC-V with multiple steps but that would just circumvent the
!flag_trapping_math check.  Then I wondered why we vectorize this
using multiple steps on x86 even with trapping math and it turns out
that the difference is the NARROW_DST modifier but we emit
VEC_PACK_FIX_TRUNC_EXPR anyway.  Is this "safe"?

Regards
 Robin
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.target/i386/pr110018-1.c b/gcc/testsuite/gcc.target/i386/pr110018-1.c
new file mode 100644
index 00000000000..b1baffd7af1
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr110018-1.c
@@ -0,0 +1,94 @@ 
+/* { dg-do compile } */
+/* { dg-options "-mavx512fp16 -mavx512vl -O2 -mavx512dq" } */
+/* { dg-final { scan-assembler-times {(?n)vcvttp[dsh]2[dqw]} 5 } } */
+/* { dg-final { scan-assembler-times {(?n)vcvt[dqw]*2p[dsh]} 5 } } */
+
+void
+foo (double* __restrict a, char* b)
+{
+  a[0] = b[0];
+  a[1] = b[1];
+}
+
+void
+foo1 (float* __restrict a, char* b)
+{
+  a[0] = b[0];
+  a[1] = b[1];
+  a[2] = b[2];
+  a[3] = b[3];
+}
+
+void
+foo2 (_Float16* __restrict a, char* b)
+{
+  a[0] = b[0];
+  a[1] = b[1];
+  a[2] = b[2];
+  a[3] = b[3];
+  a[4] = b[4];
+  a[5] = b[5];
+  a[6] = b[6];
+  a[7] = b[7];
+}
+
+void
+foo3 (double* __restrict a, short* b)
+{
+  a[0] = b[0];
+  a[1] = b[1];
+}
+
+void
+foo4 (float* __restrict a, char* b)
+{
+  a[0] = b[0];
+  a[1] = b[1];
+  a[2] = b[2];
+  a[3] = b[3];
+}
+
+void
+foo5 (double* __restrict b, char* a)
+{
+  a[0] = b[0];
+  a[1] = b[1];
+}
+
+void
+foo6 (float* __restrict b, char* a)
+{
+  a[0] = b[0];
+  a[1] = b[1];
+  a[2] = b[2];
+  a[3] = b[3];
+}
+
+void
+foo7 (_Float16* __restrict b, char* a)
+{
+  a[0] = b[0];
+  a[1] = b[1];
+  a[2] = b[2];
+  a[3] = b[3];
+  a[4] = b[4];
+  a[5] = b[5];
+  a[6] = b[6];
+  a[7] = b[7];
+}
+
+void
+foo8 (double* __restrict b, short* a)
+{
+  a[0] = b[0];
+  a[1] = b[1];
+}
+
+void
+foo9 (float* __restrict b, char* a)
+{
+  a[0] = b[0];
+  a[1] = b[1];
+  a[2] = b[2];
+  a[3] = b[3];
+}
diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
index bd3b07a3aa1..1118c89686d 100644
--- a/gcc/tree-vect-stmts.cc
+++ b/gcc/tree-vect-stmts.cc
@@ -5162,6 +5162,49 @@  vectorizable_conversion (vec_info *vinfo,
 	return false;
       if (supportable_convert_operation (code, vectype_out, vectype_in, &code1))
 	break;
+      if ((code == FLOAT_EXPR
+	   && GET_MODE_SIZE (lhs_mode) > GET_MODE_SIZE (rhs_mode))
+	  || (code == FIX_TRUNC_EXPR
+	      && GET_MODE_SIZE (rhs_mode) > GET_MODE_SIZE (lhs_mode)))
+	{
+	  bool float_expr_p = code == FLOAT_EXPR;
+	  scalar_mode imode = float_expr_p ? rhs_mode : lhs_mode;
+	  fltsz = GET_MODE_SIZE (float_expr_p ? lhs_mode : rhs_mode);
+	  code1 = float_expr_p ? code : NOP_EXPR;
+	  codecvt1 = float_expr_p ? NOP_EXPR : code;
+	  FOR_EACH_2XWIDER_MODE (rhs_mode_iter, imode)
+	    {
+	      imode = rhs_mode_iter.require ();
+	      if (GET_MODE_SIZE (imode) > fltsz)
+		break;
+
+	      cvt_type
+		= build_nonstandard_integer_type (GET_MODE_BITSIZE (imode),
+						  0);
+	      cvt_type = get_vectype_for_scalar_type (vinfo, cvt_type,
+						      slp_node);
+	      /* This should only happened for SLP as long as loop vectorizer
+		 only supports same-sized vector.  */
+	      if (cvt_type == NULL_TREE
+		  || maybe_ne (TYPE_VECTOR_SUBPARTS (cvt_type), nunits_in)
+		  || !supportable_convert_operation (code1, vectype_out,
+						     cvt_type, &code1)
+		  || !supportable_convert_operation (codecvt1, cvt_type,
+						     vectype_in, &codecvt1))
+		continue;
+
+	      found_mode = true;
+	      break;
+	    }
+
+	  if (found_mode)
+	    {
+	      multi_step_cvt++;
+	      interm_types.safe_push (cvt_type);
+	      cvt_type = NULL_TREE;
+	      break;
+	    }
+	}
       /* FALLTHRU */
     unsupported:
       if (dump_enabled_p ())
@@ -5381,7 +5424,18 @@  vectorizable_conversion (vec_info *vinfo,
 	{
 	  /* Arguments are ready, create the new vector stmt.  */
 	  gcc_assert (TREE_CODE_LENGTH (code1) == unary_op);
-	  gassign *new_stmt = gimple_build_assign (vec_dest, code1, vop0);
+	  gassign* new_stmt;
+	  if (multi_step_cvt)
+	    {
+	      gcc_assert (multi_step_cvt == 1);
+	      new_stmt = gimple_build_assign (vec_dest, codecvt1, vop0);
+	      new_temp = make_ssa_name (vec_dest, new_stmt);
+	      gimple_assign_set_lhs (new_stmt, new_temp);
+	      vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
+	      vop0 = new_temp;
+	      vec_dest = vec_dsts[0];
+	    }
+	  new_stmt = gimple_build_assign (vec_dest, code1, vop0);
 	  new_temp = make_ssa_name (vec_dest, new_stmt);
 	  gimple_assign_set_lhs (new_stmt, new_temp);
 	  vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);