* [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
@ 2023-06-02 1:00 liuhongt
2023-06-20 8:38 ` Richard Biener
0 siblings, 1 reply; 16+ messages in thread
From: liuhongt @ 2023-06-02 1:00 UTC (permalink / raw)
To: gcc-patches; +Cc: crazylht, hjl.tools
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;
+ 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);
--
2.39.1.388.g2fc9e9ca3c
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-02 1:00 [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed liuhongt
@ 2023-06-20 8:38 ` Richard Biener
2023-06-20 9:09 ` Hongtao Liu
2023-06-21 9:10 ` Richard Sandiford
0 siblings, 2 replies; 16+ messages in thread
From: Richard Biener @ 2023-06-20 8:38 UTC (permalink / raw)
To: liuhongt; +Cc: gcc-patches, crazylht, hjl.tools
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-20 8:38 ` Richard Biener
@ 2023-06-20 9:09 ` Hongtao Liu
2023-06-20 9:22 ` Richard Biener
2023-06-21 9:10 ` Richard Sandiford
1 sibling, 1 reply; 16+ messages in thread
From: Hongtao Liu @ 2023-06-20 9:09 UTC (permalink / raw)
To: Richard Biener; +Cc: liuhongt, gcc-patches, hjl.tools
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
> >
--
BR,
Hongtao
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-20 9:09 ` Hongtao Liu
@ 2023-06-20 9:22 ` Richard Biener
2023-06-20 16:11 ` liuhongt
0 siblings, 1 reply; 16+ messages in thread
From: Richard Biener @ 2023-06-20 9:22 UTC (permalink / raw)
To: Hongtao Liu; +Cc: liuhongt, gcc-patches, hjl.tools
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-20 9:22 ` Richard Biener
@ 2023-06-20 16:11 ` liuhongt
2023-06-21 7:49 ` Uros Bizjak
2023-06-22 20:36 ` Thiago Jung Bauermann
0 siblings, 2 replies; 16+ messages in thread
From: liuhongt @ 2023-06-20 16:11 UTC (permalink / raw)
To: gcc-patches; +Cc: crazylht, hjl.tools
I notice there's some refactor in vectorizable_conversion
for code_helper,so I've adjusted my patch to that.
Here's the patch I'm going to commit.
We have already use intermidate type in case WIDEN, but not for NONE,
this patch extended that.
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 | 66 ++++++++++++++-
2 files changed, 158 insertions(+), 2 deletions(-)
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 056a0ecb2be..ae24f3e66e6 100644
--- a/gcc/tree-vect-stmts.cc
+++ b/gcc/tree-vect-stmts.cc
@@ -5041,7 +5041,7 @@ vectorizable_conversion (vec_info *vinfo,
tree scalar_dest;
tree op0, op1 = NULL_TREE;
loop_vec_info loop_vinfo = dyn_cast <loop_vec_info> (vinfo);
- tree_code tc1;
+ tree_code tc1, tc2;
code_helper code, code1, code2;
code_helper codecvt1 = ERROR_MARK, codecvt2 = ERROR_MARK;
tree new_temp;
@@ -5249,6 +5249,57 @@ vectorizable_conversion (vec_info *vinfo,
code1 = tc1;
break;
}
+
+ /* 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 ((tree_code) code1,
+ vectype_out,
+ cvt_type, &tc1)
+ || !supportable_convert_operation ((tree_code) codecvt1,
+ cvt_type,
+ vectype_in, &tc2))
+ continue;
+
+ found_mode = true;
+ break;
+ }
+
+ if (found_mode)
+ {
+ multi_step_cvt++;
+ interm_types.safe_push (cvt_type);
+ cvt_type = NULL_TREE;
+ code1 = tc1;
+ codecvt1 = tc2;
+ break;
+ }
+ }
/* FALLTHRU */
unsupported:
if (dump_enabled_p ())
@@ -5513,7 +5564,18 @@ vectorizable_conversion (vec_info *vinfo,
FOR_EACH_VEC_ELT (vec_oprnds0, i, vop0)
{
/* Arguments are ready, create the new vector stmt. */
- gimple *new_stmt = vect_gimple_build (vec_dest, code1, vop0);
+ gimple* new_stmt;
+ if (multi_step_cvt)
+ {
+ gcc_assert (multi_step_cvt == 1);
+ new_stmt = vect_gimple_build (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 = vect_gimple_build (vec_dest, code1, vop0);
new_temp = make_ssa_name (vec_dest, new_stmt);
gimple_set_lhs (new_stmt, new_temp);
vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
--
2.39.1.388.g2fc9e9ca3c
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-20 16:11 ` liuhongt
@ 2023-06-21 7:49 ` Uros Bizjak
2023-06-21 8:19 ` Richard Biener
2023-06-22 20:36 ` Thiago Jung Bauermann
1 sibling, 1 reply; 16+ messages in thread
From: Uros Bizjak @ 2023-06-21 7:49 UTC (permalink / raw)
To: liuhongt; +Cc: gcc-patches, crazylht, hjl.tools
[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]
On Tue, Jun 20, 2023 at 6:11 PM liuhongt via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> I notice there's some refactor in vectorizable_conversion
> for code_helper,so I've adjusted my patch to that.
> Here's the patch I'm going to commit.
>
> We have already use intermidate type in case WIDEN, but not for NONE,
> this patch extended that.
>
> 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.
> +
> + /* For conversions between float and smaller integer types try whether we
> + can use intermediate signed integer types to support the
> + conversion. */
I'm trying to enhance testcase coverage with explicit signed/unsigned
types (patch attached), and I have noticed that zero-extension is used
for unsigned types. So, the above comment that mentions only signed
integer types is not entirely correct.
Uros.
[-- Attachment #2: t.diff.txt --]
[-- Type: text/plain, Size: 3611 bytes --]
diff --git a/gcc/testsuite/gcc.target/i386/pr110018-1.c b/gcc/testsuite/gcc.target/i386/pr110018-1.c
index b1baffd7af1..b6a3be7b7a2 100644
--- a/gcc/testsuite/gcc.target/i386/pr110018-1.c
+++ b/gcc/testsuite/gcc.target/i386/pr110018-1.c
@@ -4,14 +4,14 @@
/* { dg-final { scan-assembler-times {(?n)vcvt[dqw]*2p[dsh]} 5 } } */
void
-foo (double* __restrict a, char* b)
+foo (double* __restrict a, signed char* b)
{
a[0] = b[0];
a[1] = b[1];
}
void
-foo1 (float* __restrict a, char* b)
+foo1 (float* __restrict a, signed char* b)
{
a[0] = b[0];
a[1] = b[1];
@@ -20,7 +20,7 @@ foo1 (float* __restrict a, char* b)
}
void
-foo2 (_Float16* __restrict a, char* b)
+foo2 (_Float16* __restrict a, signed char* b)
{
a[0] = b[0];
a[1] = b[1];
@@ -33,14 +33,14 @@ foo2 (_Float16* __restrict a, char* b)
}
void
-foo3 (double* __restrict a, short* b)
+foo3 (double* __restrict a, signed short* b)
{
a[0] = b[0];
a[1] = b[1];
}
void
-foo4 (float* __restrict a, char* b)
+foo4 (float* __restrict a, signed char* b)
{
a[0] = b[0];
a[1] = b[1];
@@ -49,14 +49,14 @@ foo4 (float* __restrict a, char* b)
}
void
-foo5 (double* __restrict b, char* a)
+foo5 (double* __restrict b, signed char* a)
{
a[0] = b[0];
a[1] = b[1];
}
void
-foo6 (float* __restrict b, char* a)
+foo6 (float* __restrict b, signed char* a)
{
a[0] = b[0];
a[1] = b[1];
@@ -65,7 +65,7 @@ foo6 (float* __restrict b, char* a)
}
void
-foo7 (_Float16* __restrict b, char* a)
+foo7 (_Float16* __restrict b, signed char* a)
{
a[0] = b[0];
a[1] = b[1];
@@ -78,14 +78,14 @@ foo7 (_Float16* __restrict b, char* a)
}
void
-foo8 (double* __restrict b, short* a)
+foo8 (double* __restrict b, signed short* a)
{
a[0] = b[0];
a[1] = b[1];
}
void
-foo9 (float* __restrict b, char* a)
+foo9 (float* __restrict b, signed char* a)
{
a[0] = b[0];
a[1] = b[1];
diff --git a/gcc/testsuite/gcc.target/i386/pr110018-2.c b/gcc/testsuite/gcc.target/i386/pr110018-2.c
new file mode 100644
index 00000000000..a663e074698
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr110018-2.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, unsigned char* b)
+{
+ a[0] = b[0];
+ a[1] = b[1];
+}
+
+void
+foo1 (float* __restrict a, unsigned char* b)
+{
+ a[0] = b[0];
+ a[1] = b[1];
+ a[2] = b[2];
+ a[3] = b[3];
+}
+
+void
+foo2 (_Float16* __restrict a, unsigned 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, unsigned short* b)
+{
+ a[0] = b[0];
+ a[1] = b[1];
+}
+
+void
+foo4 (float* __restrict a, unsigned char* b)
+{
+ a[0] = b[0];
+ a[1] = b[1];
+ a[2] = b[2];
+ a[3] = b[3];
+}
+
+void
+foo5 (double* __restrict b, unsigned char* a)
+{
+ a[0] = b[0];
+ a[1] = b[1];
+}
+
+void
+foo6 (float* __restrict b, unsigned char* a)
+{
+ a[0] = b[0];
+ a[1] = b[1];
+ a[2] = b[2];
+ a[3] = b[3];
+}
+
+void
+foo7 (_Float16* __restrict b, unsigned 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, unsigned short* a)
+{
+ a[0] = b[0];
+ a[1] = b[1];
+}
+
+void
+foo9 (float* __restrict b, unsigned char* a)
+{
+ a[0] = b[0];
+ a[1] = b[1];
+ a[2] = b[2];
+ a[3] = b[3];
+}
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-21 7:49 ` Uros Bizjak
@ 2023-06-21 8:19 ` Richard Biener
2023-06-21 14:37 ` Uros Bizjak
0 siblings, 1 reply; 16+ messages in thread
From: Richard Biener @ 2023-06-21 8:19 UTC (permalink / raw)
To: Uros Bizjak; +Cc: liuhongt, gcc-patches, crazylht, hjl.tools
On Wed, Jun 21, 2023 at 9:50 AM Uros Bizjak via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Tue, Jun 20, 2023 at 6:11 PM liuhongt via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > I notice there's some refactor in vectorizable_conversion
> > for code_helper,so I've adjusted my patch to that.
> > Here's the patch I'm going to commit.
> >
> > We have already use intermidate type in case WIDEN, but not for NONE,
> > this patch extended that.
> >
> > 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.
>
> > +
> > + /* For conversions between float and smaller integer types try whether we
> > + can use intermediate signed integer types to support the
> > + conversion. */
>
> I'm trying to enhance testcase coverage with explicit signed/unsigned
> types (patch attached), and I have noticed that zero-extension is used
> for unsigned types. So, the above comment that mentions only signed
> integer types is not entirely correct.
The comment says the intermediate sized vector types are always
signed (because float conversions to/from unsigned are always somewhat
awkward), but yes, if the original type was unsigned zero-extension is
used and if it was signed sign-extension.
The testcase adjustments / additions look good to me btw.
Thanks,
Richard.
>
> Uros.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-20 8:38 ` Richard Biener
2023-06-20 9:09 ` Hongtao Liu
@ 2023-06-21 9:10 ` Richard Sandiford
2023-06-21 9:32 ` Richard Sandiford
1 sibling, 1 reply; 16+ messages in thread
From: Richard Sandiford @ 2023-06-21 9:10 UTC (permalink / raw)
To: Richard Biener via Gcc-patches
Cc: liuhongt, Richard Biener, crazylht, hjl.tools
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
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-21 9:10 ` Richard Sandiford
@ 2023-06-21 9:32 ` Richard Sandiford
2023-06-21 11:04 ` Richard Biener
0 siblings, 1 reply; 16+ messages in thread
From: Richard Sandiford @ 2023-06-21 9:32 UTC (permalink / raw)
To: Richard Biener via Gcc-patches
Cc: liuhongt, Richard Biener, crazylht, hjl.tools
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-21 9:32 ` Richard Sandiford
@ 2023-06-21 11:04 ` Richard Biener
2023-06-21 11:20 ` Richard Sandiford
2023-06-21 20:39 ` Joseph Myers
0 siblings, 2 replies; 16+ messages in thread
From: Richard Biener @ 2023-06-21 11:04 UTC (permalink / raw)
To: Richard Biener via Gcc-patches, liuhongt, Richard Biener,
crazylht, hjl.tools, richard.sandiford
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-21 11:04 ` Richard Biener
@ 2023-06-21 11:20 ` Richard Sandiford
2023-06-21 20:39 ` Joseph Myers
1 sibling, 0 replies; 16+ messages in thread
From: Richard Sandiford @ 2023-06-21 11:20 UTC (permalink / raw)
To: Richard Biener
Cc: Richard Biener via Gcc-patches, liuhongt, crazylht, hjl.tools
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-21 8:19 ` Richard Biener
@ 2023-06-21 14:37 ` Uros Bizjak
0 siblings, 0 replies; 16+ messages in thread
From: Uros Bizjak @ 2023-06-21 14:37 UTC (permalink / raw)
To: Richard Biener; +Cc: liuhongt, gcc-patches, crazylht, hjl.tools
On Wed, Jun 21, 2023 at 10:22 AM Richard Biener
<richard.guenther@gmail.com> wrote:
> > > + /* For conversions between float and smaller integer types try whether we
> > > + can use intermediate signed integer types to support the
> > > + conversion. */
> >
> > I'm trying to enhance testcase coverage with explicit signed/unsigned
> > types (patch attached), and I have noticed that zero-extension is used
> > for unsigned types. So, the above comment that mentions only signed
> > integer types is not entirely correct.
>
> The comment says the intermediate sized vector types are always
> signed (because float conversions to/from unsigned are always somewhat
> awkward), but yes, if the original type was unsigned zero-extension is
> used and if it was signed sign-extension.
>
> The testcase adjustments / additions look good to me btw.
Thanks, pushed with the following ChangeLog:
vect: Add testcases for unsigned conversions [PR110018]
Also test conversions with unsigned types.
PR target/110018
gcc/testsuite/ChangeLog:
* gcc.target/i386/pr110018-1.c: Use explicit signed types.
* gcc.target/i386/pr110018-2.c: New test.
Tested on x86_64-linux-gnu {,-m32}.
Uros.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-21 11:04 ` Richard Biener
2023-06-21 11:20 ` Richard Sandiford
@ 2023-06-21 20:39 ` Joseph Myers
2023-06-22 7:32 ` Richard Biener
1 sibling, 1 reply; 16+ messages in thread
From: Joseph Myers @ 2023-06-21 20:39 UTC (permalink / raw)
To: Richard Biener
Cc: Richard Biener via Gcc-patches, liuhongt, crazylht, hjl.tools,
richard.sandiford
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).
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-21 20:39 ` Joseph Myers
@ 2023-06-22 7:32 ` Richard Biener
2023-07-12 9:19 ` Robin Dapp
0 siblings, 1 reply; 16+ messages in thread
From: Richard Biener @ 2023-06-22 7:32 UTC (permalink / raw)
To: Joseph Myers
Cc: Richard Biener via Gcc-patches, liuhongt, crazylht, hjl.tools,
richard.sandiford
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-20 16:11 ` liuhongt
2023-06-21 7:49 ` Uros Bizjak
@ 2023-06-22 20:36 ` Thiago Jung Bauermann
1 sibling, 0 replies; 16+ messages in thread
From: Thiago Jung Bauermann @ 2023-06-22 20:36 UTC (permalink / raw)
To: liuhongt; +Cc: crazylht, hjl.tools, gcc-patches
Hello,
liuhongt via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> I notice there's some refactor in vectorizable_conversion
> for code_helper,so I've adjusted my patch to that.
> Here's the patch I'm going to commit.
>
> We have already use intermidate type in case WIDEN, but not for NONE,
> this patch extended that.
>
> 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 | 66 ++++++++++++++-
> 2 files changed, 158 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/i386/pr110018-1.c
Our CI detected regressions on aarch64-linux-gnu with this commit, in
some aarch64-sve and gfortran tests. I filed the following bug report
with the details:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110371
Could you please check?
--
Thiago
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed.
2023-06-22 7:32 ` Richard Biener
@ 2023-07-12 9:19 ` Robin Dapp
0 siblings, 0 replies; 16+ messages in thread
From: Robin Dapp @ 2023-07-12 9:19 UTC (permalink / raw)
To: Richard Biener, Joseph Myers
Cc: rdapp.gcc, Richard Biener via Gcc-patches, liuhongt, crazylht,
hjl.tools, richard.sandiford
>>>>> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-07-12 9:19 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-02 1:00 [PATCH] [vect]Use intermiediate integer type for float_expr/fix_trunc_expr when direct optab is not existed liuhongt
2023-06-20 8:38 ` Richard Biener
2023-06-20 9:09 ` Hongtao Liu
2023-06-20 9:22 ` Richard Biener
2023-06-20 16:11 ` liuhongt
2023-06-21 7:49 ` Uros Bizjak
2023-06-21 8:19 ` Richard Biener
2023-06-21 14:37 ` Uros Bizjak
2023-06-22 20:36 ` Thiago Jung Bauermann
2023-06-21 9:10 ` Richard Sandiford
2023-06-21 9:32 ` Richard Sandiford
2023-06-21 11:04 ` Richard Biener
2023-06-21 11:20 ` Richard Sandiford
2023-06-21 20:39 ` Joseph Myers
2023-06-22 7:32 ` Richard Biener
2023-07-12 9:19 ` Robin Dapp
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).