From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27957 invoked by alias); 4 Aug 2015 12:15:39 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 27927 invoked by uid 89); 4 Aug 2015 12:15:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_50,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Tue, 04 Aug 2015 12:15:36 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 23E5DAD82; Tue, 4 Aug 2015 12:15:33 +0000 (UTC) Date: Tue, 04 Aug 2015 12:15:00 -0000 From: Richard Biener To: Petr Murzin cc: Kirill Yukhin , "gcc-patches@gcc.gnu.org" , ubizjak@gmail.com Subject: Re: [PATCH] [AVX512F] Add scatter support for vectorizer In-Reply-To: Message-ID: References: User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2015-08/txt/msg00174.txt.bz2 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 > Petr Murzin > > 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 SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)