On Tue, 12 May 2026, Cui, Lili wrote: > > > > -----Original Message----- > > From: Richard Biener <[email protected]> > > Sent: Monday, May 11, 2026 8:17 PM > > To: Cui, Lili <[email protected]> > > Cc: [email protected]; Liu, Hongtao <[email protected]> > > Subject: Re: [PATCH 1/2] tree-optimization/vect: Allow commutative operand > > swap for IFN in SLP reduction > > > > > --- a/gcc/tree-vect-slp.cc > > > +++ b/gcc/tree-vect-slp.cc > > > @@ -1401,7 +1401,14 @@ vect_build_slp_tree_1 (vec_info *vinfo, unsigned > > char *swap, > > > && STMT_VINFO_REDUC_IDX (stmt_info) != -1 > > > && rhs_code.is_tree_code () > > > && commutative_tree_code (tree_code (rhs_code)) > > > - && first_reduc_idx == 1 - STMT_VINFO_REDUC_IDX > > (stmt_info))) > > > + && first_reduc_idx == 1 - STMT_VINFO_REDUC_IDX > > (stmt_info)) > > > + && !(first_reduc_idx != -1 > > > + && STMT_VINFO_REDUC_IDX (stmt_info) != -1 > > > + && rhs_code.is_internal_fn () > > > + && first_commutative_argument (internal_fn (rhs_code)) >= > > 0 > > > + && (first_reduc_idx + STMT_VINFO_REDUC_IDX (stmt_info) > > > + == 2 * first_commutative_argument ( > > > + internal_fn (rhs_code)) + 1))) > > > > I suppose > > > > first_reduc_idx == first_commutative_argument (...) + 1 > > - STMT_VINFO_REDUC_IDX (stmt_info) > > > > would have worked as well. These are basically sanity checks that the > > indexes > > are either equal or the current one is "the other". > > > > Note that there is first_commutative_argument (code_helper, type) that we > > could have used to unify the tree-code and internal-fn paths. 'type' would > > be > > the return type aka TREE_TYPE (lhs). > > > > The patch is OK, but it would be nice to do the unification. > > > Hi Richard, > > Thanks for the review and suggestions! > > You're right. I've simplified the implementation to use the unified > first_commutative_argument(code_helper, tree) interface, which handles both > tree codes and internal functions in one path. > > The updated check now looks like: > > @@ -1391,6 +1391,7 @@ vect_build_slp_tree_1 (vec_info *vinfo, unsigned char > *swap, > } > else > { > + int comm_arg; > if (first_reduc_idx != STMT_VINFO_REDUC_IDX (stmt_info) > /* For SLP reduction groups the index isn't necessarily > uniform but only that of the first stmt matters. */ > @@ -1399,9 +1400,9 @@ vect_build_slp_tree_1 (vec_info *vinfo, unsigned char > *swap, > && REDUC_GROUP_FIRST_ELEMENT (stmt_info)) > && !(first_reduc_idx != -1 > && STMT_VINFO_REDUC_IDX (stmt_info) != -1 > - && rhs_code.is_tree_code () > - && commutative_tree_code (tree_code (rhs_code)) > - && first_reduc_idx == 1 - STMT_VINFO_REDUC_IDX > (stmt_info))) > + && (comm_arg = first_commutative_argument > + (rhs_code, TREE_TYPE (gimple_get_lhs > (stmt)))) >= 0
I think 'lhs' is already available and populated > + && (first_reduc_idx == 2 * comm_arg + 1 - > STMT_VINFO_REDUC_IDX (stmt_info)))) > > This unifies the tree-code and IFN paths. The formula first_reduc_idx == 2 * > comm_arg + 1 - STMT_VINFO_REDUC_IDX verifies that the two indices are either > equal or represent "the other" commutative operand position: > > For tree codes (PLUS_EXPR, etc.): > - comm_arg = 0 > - Formula: first_reduc_idx == 1 - STMT_VINFO_REDUC_IDX > - Example: reduc_idx 0 and 1 satisfy 0 == 1-1 or 1 == 1-0 ✓ > > For internal functions (.COND_ADD, etc.): > - comm_arg = 1 > - Formula: first_reduc_idx == 3 - STMT_VINFO_REDUC_IDX > - Example: .COND_ADD(cond, arg1, arg2, else) where reduc_idx 1 and 2 > satisfy 1 == 3-2 or 2 == 3-1 ✓ > > Bootstrapped and retested on x86_64-linux-gnu. Performance improvements not > changed. > > Does this look better? Yes, this looks good besides the 'lhs' issue. OK with re-using lhs. Thanks, Richard. > Thanks, > Lili > > > > Thanks, > > Richard. > -- Richard Biener <[email protected]> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Jochen Jaser, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
