From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 79A82396E879 for ; Fri, 27 May 2022 13:23:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 79A82396E879 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 4C9C91F889; Fri, 27 May 2022 13:23:56 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 3AC582C141; Fri, 27 May 2022 13:23:56 +0000 (UTC) Date: Fri, 27 May 2022 13:23:56 +0000 (UTC) From: Richard Biener To: Joel Hutton cc: Richard Sandiford , "gcc-patches@gcc.gnu.org" Subject: Re: [ping][vect-patterns] Refactor widen_plus/widen_minus as internal_fns In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 May 2022 13:23:59 -0000 On Wed, 25 May 2022, Joel Hutton wrote: > Ping! > > Just checking there is still interest in this. I'm assuming you've been > busy with release. Can you post an updated patch (after the .cc renaming, and code_helper now already moved to tree.h). Thanks, Richard. > Joel > > > -----Original Message----- > > From: Joel Hutton > > Sent: 13 April 2022 16:53 > > To: Richard Sandiford > > Cc: Richard Biener ; gcc-patches@gcc.gnu.org > > Subject: [vect-patterns] Refactor widen_plus/widen_minus as internal_fns > > > > Hi all, > > > > These patches refactor the widening patterns in vect-patterns to use > > internal_fn instead of tree_codes. > > > > Sorry about the delay, some changes to master made it a bit messier. > > > > Bootstrapped and regression tested on aarch64. > > > > Joel > > > > > > diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c > > > > index 854cbcff390..4a8ea67e62f 100644 > > > > --- a/gcc/tree-vect-patterns.c > > > > +++ b/gcc/tree-vect-patterns.c > > > > @@ -1245,7 +1245,7 @@ vect_recog_sad_pattern (vec_info *vinfo, > > > > static gimple * vect_recog_widen_op_pattern (vec_info *vinfo, > > > > stmt_vec_info last_stmt_info, tree *type_out, > > > > - tree_code orig_code, tree_code wide_code, > > > > + tree_code orig_code, code_helper > > > wide_code_or_ifn, > > > > > > I think it'd be better to keep the original ?wide_code? name and try > > > to remove as many places as possible in which switching based on > > > tree_code or internal_fn is necessary. The recent gimple-match.h > > > patches should help with that, but more routines might be needed. > > > > Done. > > > > > > @@ -1309,8 +1310,16 @@ vect_recog_widen_op_pattern (vec_info > > *vinfo, > > > > 2, oprnd, half_type, unprom, vectype); > > > > > > > > tree var = vect_recog_temp_ssa_var (itype, NULL); > > > > - gimple *pattern_stmt = gimple_build_assign (var, wide_code, > > > > - oprnd[0], oprnd[1]); > > > > + gimple *pattern_stmt; > > > > + if (wide_code_or_ifn.is_tree_code ()) > > > > + pattern_stmt = gimple_build_assign (var, wide_code_or_ifn, > > > > + oprnd[0], oprnd[1]); > > > > + else > > > > + { > > > > + internal_fn fn = as_internal_fn ((combined_fn) wide_code_or_ifn); > > > > + pattern_stmt = gimple_build_call_internal (fn, 2, oprnd[0], oprnd[1]); > > > > + gimple_call_set_lhs (pattern_stmt, var); > > > > + } > > > > > > For example, I think we should hide this inside a new: > > > > > > gimple_build (var, wide_code, oprnd[0], oprnd[1]); > > > > > > that works directly on code_helper, similarly to the new code_helper > > > gimple_build interfaces. > > > > Done. > > > > > > @@ -4513,14 +4513,16 @@ vect_gen_widened_results_half (vec_info > > > *vinfo, enum tree_code code, > > > > tree new_temp; > > > > > > > > /* Generate half of the widened result: */ > > > > - gcc_assert (op_type == TREE_CODE_LENGTH (code)); > > > > if (op_type != binary_op) > > > > vec_oprnd1 = NULL; > > > > - new_stmt = gimple_build_assign (vec_dest, code, vec_oprnd0, > > > vec_oprnd1); > > > > + if (ch.is_tree_code ()) > > > > + new_stmt = gimple_build_assign (vec_dest, ch, vec_oprnd0, > > > vec_oprnd1); > > > > + else > > > > + new_stmt = gimple_build_call_internal (as_internal_fn > > > > + ((combined_fn) > > > ch), > > > > + 2, vec_oprnd0, vec_oprnd1); > > > > > > Similarly here. I guess the combined_fn/internal_fn path will also > > > need to cope with null trailing operands, for consistency with the tree_code > > one. > > > > > > > Done. > > > > > > @@ -4744,31 +4747,28 @@ vectorizable_conversion (vec_info *vinfo, > > > > && ! vec_stmt) > > > > return false; > > > > > > > > - gassign *stmt = dyn_cast (stmt_info->stmt); > > > > - if (!stmt) > > > > + gimple* stmt = stmt_info->stmt; > > > > + if (!(is_gimple_assign (stmt) || is_gimple_call (stmt))) > > > > return false; > > > > > > > > - if (TREE_CODE (gimple_assign_lhs (stmt)) != SSA_NAME) > > > > - return false; > > > > + if (is_gimple_assign (stmt)) > > > > + { > > > > + code_or_ifn = gimple_assign_rhs_code (stmt); } else > > > > + code_or_ifn = gimple_call_combined_fn (stmt); > > > > > > It might be possible to use gimple_extract_op here (only recently added). > > > This would also provide the number of operands directly, instead of > > > needing ?op_type?. It would also provide an array of operands. > > > > > > > Done. > > > > > > - code = gimple_assign_rhs_code (stmt); > > > > - if (!CONVERT_EXPR_CODE_P (code) > > > > - && code != FIX_TRUNC_EXPR > > > > - && code != FLOAT_EXPR > > > > - && code != WIDEN_PLUS_EXPR > > > > - && code != WIDEN_MINUS_EXPR > > > > - && code != WIDEN_MULT_EXPR > > > > - && code != WIDEN_LSHIFT_EXPR) > > > > > > Is it safe to drop this check independently of parts 2 and 3? > > > (Genuine question, haven't checked in detail.) > > > > It requires the parts 2 and 3. I've moved that change into this first patch. > > > > > > @@ -4784,7 +4784,8 @@ vectorizable_conversion (vec_info *vinfo, > > > > } > > > > > > > > rhs_type = TREE_TYPE (op0); > > > > - if ((code != FIX_TRUNC_EXPR && code != FLOAT_EXPR) > > > > + if ((code_or_ifn.is_tree_code () && code_or_ifn != FIX_TRUNC_EXPR > > > > + && code_or_ifn != FLOAT_EXPR) > > > > > > I don't think we want the is_tree_code condition here. The existing > > > != should work. > > > > > > > Done. > > > > > > @@ -11856,13 +11888,13 @@ supportable_widening_operation > > (vec_info > > > *vinfo, > > > > if (BYTES_BIG_ENDIAN && c1 != VEC_WIDEN_MULT_EVEN_EXPR) > > > > std::swap (c1, c2); > > > > > > > > - if (code == FIX_TRUNC_EXPR) > > > > + if (code_or_ifn == FIX_TRUNC_EXPR) > > > > { > > > > /* The signedness is determined from output operand. */ > > > > optab1 = optab_for_tree_code (c1, vectype_out, optab_default); > > > > optab2 = optab_for_tree_code (c2, vectype_out, optab_default); > > > > } > > > > - else if (CONVERT_EXPR_CODE_P (code) > > > > + else if (CONVERT_EXPR_CODE_P ((tree_code) code_or_ifn) > > > > > > I think this should be as_tree_code (), so that it's safe for internal > > > functions if (tree_code) ever becomes a checked convrsion in future. > > > Same for other instances. > > > > > > > Done. > > > > > > && VECTOR_BOOLEAN_TYPE_P (wide_vectype) > > > > && VECTOR_BOOLEAN_TYPE_P (vectype) > > > > && TYPE_MODE (wide_vectype) == TYPE_MODE (vectype) [?] @@ > > > > -12000,7 +12031,7 @@ supportable_widening_operation (vec_info > > > *vinfo, > > > > bool > > > > supportable_narrowing_operation (enum tree_code code, > > > > tree vectype_out, tree vectype_in, > > > > - enum tree_code *code1, int *multi_step_cvt, > > > > + void* _code1, int *multi_step_cvt, > > > > > > This might be rehashing an old conversation, sorry, but why does this > > > need to be void? > > > > > > > Reworked to avoid using void*. > > > > > > vec *interm_types) { > > > > machine_mode vec_mode; > > > > @@ -12013,6 +12044,7 @@ supportable_narrowing_operation (enum > > > tree_code code, > > > > machine_mode intermediate_mode, prev_mode; > > > > int i; > > > > bool uns; > > > > + tree_code * code1 = (tree_code*) _code1; > > > > > > > > *multi_step_cvt = 0; > > > > switch (code) > > > > diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h index > > > > bd6f334d15f..70c06264c11 100644 > > > > --- a/gcc/tree-vectorizer.h > > > > +++ b/gcc/tree-vectorizer.h > > > > @@ -2030,13 +2030,16 @@ extern bool vect_is_simple_use (vec_info *, > > > stmt_vec_info, slp_tree, > > > > enum vect_def_type *, > > > > tree *, stmt_vec_info * = NULL); extern bool > > > > vect_maybe_update_slp_op_vectype (slp_tree, tree); -extern bool > > > > supportable_widening_operation (vec_info *, > > > > - enum tree_code, stmt_vec_info, > > > > - tree, tree, enum tree_code *, > > > > - enum tree_code *, int *, > > > > - vec *); > > > > +extern bool supportable_widening_operation (vec_info *vinfo, > > > > + code_helper code_or_ifn, > > > > + stmt_vec_info stmt_info, > > > > + tree vectype_out, tree vectype_in, > > > > + code_helper *code_or_ifn1, > > > > + code_helper *code_or_ifn2, > > > > + int *multi_step_cvt, > > > > + vec *interm_types); > > > > > > Normal style is to keep the variable names out of the header. > > > The documentation lives in the .c file, so in practice, anyone who > > > wants to add a new caller will need to look there anyway. > > > > > > Thanks, > > > Richard > > > > > > > extern bool supportable_narrowing_operation (enum tree_code, tree, > > > tree, > > > > - enum tree_code *, int *, > > > > + void *, int *, > > > > vec *); > > > > > > > > extern unsigned record_stmt_cost (stmt_vector_for_cost *, int, diff > > > > --git a/gcc/tree.h b/gcc/tree.h index f62c00bc870..346565f84ce > > > > 100644 > > > > --- a/gcc/tree.h > > > > +++ b/gcc/tree.h > > > > @@ -6546,5 +6546,31 @@ extern unsigned fndecl_dealloc_argno (tree); > > > > if nonnull, set the second argument to the referenced enclosing > > > > object or pointer. Otherwise return null. */ extern tree > > > > get_attr_nonstring_decl (tree, tree * = NULL); > > > > +/* Helper to transparently allow tree codes and builtin function codes > > > > + exist in one storage entity. */ class code_helper { > > > > +public: > > > > + code_helper () {} > > > > + code_helper (tree_code code) : rep ((int) code) {} > > > > + code_helper (combined_fn fn) : rep (-(int) fn) {} > > > > + operator tree_code () const { return is_tree_code () ? > > > > + (tree_code) rep : > > > > + ERROR_MARK; } > > > > + operator combined_fn () const { return is_fn_code () ? > > > > + (combined_fn) -rep: > > > > + CFN_LAST; } > > > > + bool is_tree_code () const { return rep > 0; } > > > > + bool is_fn_code () const { return rep < 0; } > > > > + int get_rep () const { return rep; } > > > > + > > > > + enum tree_code as_tree_code () const { return is_tree_code () ? > > > > + (tree_code)* this : MAX_TREE_CODES; } combined_fn as_fn_code > > > > + () const { return is_fn_code () ? (combined_fn) > > > *this > > > > + : CFN_LAST;} > > > > + > > > > +private: > > > > + int rep; > > > > +}; > > > > > > > > #endif /* GCC_TREE_H */ > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)