> -----Original Message----- > From: Richard Biener <[email protected]> > Sent: Tuesday, May 12, 2026 3:53 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 > > 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. > Fixed and committed. I've updated the code to use the existing 'lhs' variable instead of calling gimple_get_lhs(stmt).
Thanks, Lili. > 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)
