On Fri, 31 Jul 2015, Petr Murzin wrote:
> Hello,
> This patch adds scatter support for vectorizer (for AVX512F
> instructions). Please have a look. Is it OK for trunk?
+/* Target builtin that implements vector scatter operation. */
+DEFHOOK
+(builtin_scatter,
+ "",
+ tree,
+ (const_tree vectype, const_tree index_type, int scale),
+ NULL)
please add documentation inline here, like for builtin_gather,
and let tm.texi be auto-populated.
Note that the i386 changes need target maintainer approval, CCing
Uros.
diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c
index 731fe7d..2de0369 100644
--- a/gcc/tree-vect-data-refs.c
+++ b/gcc/tree-vect-data-refs.c
@@ -65,6 +65,8 @@ along with GCC; see the file COPYING3. If not see
#include "builtins.h"
#include "params.h"
+
+
/* Return true if load- or store-lanes optab OPTAB is implemented for
COUNT vectors of type VECTYPE. NAME is the name of OPTAB. */
please avoid this kind of spurious whitespace changes.
@@ -2307,10 +2313,7 @@ vect_analyze_data_ref_access (struct data_reference
*dr)
if (dump_enabled_p ())
dump_printf_loc (MSG_NOTE, vect_location,
"zero step in outer loop.\n");
- if (DR_IS_READ (dr))
- return true;
- else
- return false;
+ return (DR_IS_READ (dr)) ? true : false;
}
}
Likewise. If anything then do
return DR_IS_READ (dr);
- if (gather)
+ if (gather || scatter)
{
tree off;
- gather = 0 != vect_check_gather (stmt, loop_vinfo, NULL, &off,
NULL);
- if (gather
+ gather = 0 != vect_check_gather_scatter (stmt, loop_vinfo, NULL,
&off, NULL, true);
+ scatter = 0 != vect_check_gather_scatter (stmt, loop_vinfo,
NULL, &off, NULL, false);
+
please only check gather/scatter once - only one, gather or scatter
can ever be true. This also means that the idea of having both
bools is not reflecting the state in a very good way. Instead
please add a
enum { SG_NONE, SCATTER, GATHER } gatherscatter;
and replace 'gather' with it.
@@ -3747,7 +3767,9 @@ again:
datarefs[i] = dr;
STMT_VINFO_GATHER_P (stmt_info) = true;
+ STMT_VINFO_SCATTER_P (stmt_info) = true;
}
this looks bougs as well due to the mechanical change - a stmt
cannot be gather and scatter at the same time.
- tree decl = vect_check_gather (stmt, loop_vinfo, NULL, &off,
NULL);
+ tree decl = vect_check_gather_scatter (stmt, loop_vinfo, NULL,
&off, NULL,
+ (STMT_VINFO_GATHER_P
(stmt_vinfo)) ? true : false);
watch long lines
if (!process_use (stmt, off, loop_vinfo, live_p, relevant,
&worklist, true))
- return false;
+ {
+ if (STMT_VINFO_SCATTER_P (stmt_vinfo) &&
+ !process_use (stmt, gimple_assign_rhs1 (stmt),
loop_vinfo, live_p,
+ relevant, &worklist, true))
+ worklist.release();
+
+ return false;
+ }
no need to cut-off the early return, no? Also rhs1 should be
already handled via
FOR_EACH_PHI_OR_STMT_USE (use_p, stmt, iter, SSA_OP_USE)
{
tree op = USE_FROM_PTR (use_p);
if (!process_use (stmt, op, loop_vinfo, live_p, relevant,
&worklist, false))
return false;
}
note that 'force' doesn't apply here.
I wonder why vect_check_gather_scatter cannot figure out itself
whether scatter or gather is used. After all it does
struct data_reference *dr = STMT_VINFO_DATA_REF (stmt_info);
so DR_IS_READ/WRITE is readily available. Please rework accordingly.
This should also simplify the patch.
+ if (!vect_is_simple_use (gimple_assign_rhs1 (stmt), NULL,
loop_vinfo, bb_vinfo,
+ &def_stmt, &def, &scatter_src_dt))
+ {
+ if (dump_enabled_p ())
+ dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
+ "scatter source use not simple.");
+ return false;
+ }
This is redundant, it is verified earlier.
+ var = make_ssa_name (var, NULL);
make_ssa_name (var);
+ new_stmt
+ = gimple_build_assign (var, VIEW_CONVERT_EXPR,
+ src, NULL_TREE);
you can omit the NULL_TREE
@@ -5586,8 +5770,6 @@ vectorizable_store (gimple stmt,
gimple_stmt_iterator *gsi, gimple *vec_stmt,
prev_stmt_info = NULL;
for (j = 0; j < ncopies; j++)
{
- gimple new_stmt;
-
if (j == 0)
{
if (slp)
spurious change?
@@ -5853,10 +6035,12 @@ permute_vec_elements (tree x, tree y, tree
mask_vec, gimple stmt,
{
tree vectype = TREE_TYPE (x);
tree perm_dest, data_ref;
+ tree scalar_dest = TREE_CODE (gimple_assign_lhs (stmt)) == SSA_NAME
+ ? gimple_assign_lhs (stmt) : x;
please instead rework vect_create_destination_var to remove the
assert this triggers for non-SSA names.
- perm_dest = vect_create_destination_var (gimple_get_lhs (stmt),
vectype);
- data_ref = make_ssa_name (perm_dest);
+ perm_dest = vect_create_destination_var (scalar_dest, vectype);
+ data_ref = make_ssa_name (perm_dest, NULL);
spurious (bad) change.
@@ -652,6 +652,9 @@ typedef struct _stmt_vec_info {
/* True if this is an access with loop-invariant stride. */
bool strided_p;
+ /* For stores only, true if this is a scatter store. */
+ bool scatter_p;
+
it can be only scatter or gather, so IMHO unifying the flags
makes sense. So
/* For stores if this is a scatter, for loads if this is a gather. */
bool scatter_gather_p;
@@ -669,6 +672,8 @@ typedef struct _stmt_vec_info {
#define STMT_VINFO_DATA_REF(S) (S)->data_ref_info
#define STMT_VINFO_GATHER_P(S) (S)->gather_p
#define STMT_VINFO_STRIDED_P(S) (S)->strided_p
+#define STMT_VINFO_STRIDE_LOAD_P(S) (S)->stride_load_p
+#define STMT_VINFO_SCATTER_P(S) (S)->scatter_p
spurious change.
Thanks,
Richard.
> Thanks,
> Petr
>
>
> 2015-07-31 Andrey Turetskiy <andrey.turets...@intel.com>
> Petr Murzin <petr.mur...@intel.com>
>
> gcc/
>
> * config/i386/i386-builtin-types.def
> (VOID_PFLOAT_HI_V8DI_V16SF_INT): New.
> (VOID_PDOUBLE_QI_V16SI_V8DF_INT): Ditto.
> (VOID_PINT_HI_V8DI_V16SI_INT): Ditto.
> (VOID_PLONGLONG_QI_V16SI_V8DI_INT): Ditto.
> * config/i386/i386.c
> (ix86_builtins): Add IX86_BUILTIN_SCATTERALTSIV8DF,
> IX86_BUILTIN_SCATTERALTDIV16SF, IX86_BUILTIN_SCATTERALTSIV8DI,
> IX86_BUILTIN_SCATTERALTDIV16SI.
> (ix86_init_mmx_sse_builtins): Define __builtin_ia32_scatteraltsiv8df,
> __builtin_ia32_scatteraltdiv8sf, __builtin_ia32_scatteraltsiv8di,
> __builtin_ia32_scatteraltdiv8si.
> (ix86_expand_builtin): Handle IX86_BUILTIN_SCATTERALTSIV8DF,
> IX86_BUILTIN_SCATTERALTDIV16SF, IX86_BUILTIN_SCATTERALTSIV8DI,
> IX86_BUILTIN_SCATTERALTDIV16SI.
> (ix86_vectorize_builtin_scatter): New.
> (TARGET_VECTORIZE_BUILTIN_SCATTER): Define as
> ix86_vectorize_builtin_scatter.
> * doc/tm.texi.in (TARGET_VECTORIZE_BUILTIN_SCATTER): New.
> * doc/tm.texi: Regenerate.
> * target.def: Add scatter builtin.
> * tree-vect-data-refs.c (vect_analyze_data_ref_dependence): Add new
> checkings for STMT_VINFO_SCATTER_P.
> (vect_check_gather): Rename to ...
> (vect_check_gather_scatter): this and enhance number of arguments.
> (vect_analyze_data_refs): Add scatter and maybe_scatter variables and
> new checkings for it accordingly.
> * tree-vectorizer.h (STMT_VINFO_SCATTER_P(S)): Define.
> (STMT_VINFO_STRIDE_LOAD_P(S)): Ditto.
> (vect_check_gather): Rename to ...
> (vect_check_gather_scatter): this.
> * triee-vect-stmts.c (vectorizable_mask_load_store): Ditto.
> (vectorizable_store): Add checkings for STMT_VINFO_SCATTER_P.
> (vect_mark_stmts_to_be_vectorized): Ditto.
>
> gcc/testsuite/
>
> * gcc.target/i386/avx512f-scatter-1.c: New.
> * gcc.target/i386/avx512f-scatter-2.c: Ditto.
> * gcc.target/i386/avx512f-scatter-3.c: Ditto.
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham
Norton, HRB 21284 (AG Nuernberg)
