On Thu, 2 Jul 2026, Zhongyao Chen wrote: > On Wed, Jul 1, 2026 at 7:56 PM Richard Biener <[email protected]> wrote: > > Hmm, the testcase is a bit complicated since there's loop > > vectorization as well. I have tried the following alternative > > patch but it doesn't make a difference for me? But in essence > > what you are testing is whether both operand0 and operand1 appear > > unform but with different enough operation so that swapping would > > make it non-uniform. So I'd have expected the following to cover > > that case as well, but maybe I misunderstood. I think we want to > > avoid creating another "do stmts match" computation, re-using > > vect_build_slp_tree_1 would be more reasonable for this (we could > > think of refactoring that a bit, of course). > > > > That said, I do see how the heuristic can be bad, and peeking at > > operand1 looks reasonable. > > > > diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc > > index 2250f6f74a1..c529f9c3f11 100644 > > --- a/gcc/tree-vect-slp.cc > > +++ b/gcc/tree-vect-slp.cc > > @@ -3013,6 +3013,18 @@ out: > > if (j != 0 && !stmt_can_swap[j]) > > can_swap_nonmatching = false; > > } > > + unsigned char *swapt = XALLOCAVEC (unsigned char, group_size); > > + poly_uint64 max_nunitst; > > + bool *matchest = XALLOCAVEC (bool, group_size); > > + bool two_operatorst = false; > > + tree vectypet = NULL_TREE; > > + if (can_swap_nonmatching > > + && vect_build_slp_tree_1 (vinfo, swapt, > > + oprnds_info[1]->def_stmts, > > + group_size, &max_nunitst, > > + matchest, &two_operatorst, > > + &vectypet)) > > + can_swap_nonmatching = false; > > } > > > > old_swap_distance = least_upthread_swappable_op_distance; > > The reason seems to be that vect_build_slp_tree_1 returns false for > operand1 here. The operand1 subtree also cannot be built as an > internal SLP node, because it hits the same kind of BIT_FIELD_REF > mismatch: > > Build SLP for stmp_rsum00_88.79_303 = > BIT_FIELD_REF <vect__15.78_302, 64, 0>; > Build SLP for stmp_isum00_89.75_296 = > BIT_FIELD_REF <vect__18.74_295, 64, 0>; > Build SLP failed: different BIT_FIELD_REF arguments in > stmp_isum00_89.75_296 = BIT_FIELD_REF <vect__18.74_295, 64, 0>;
Hmm, but then why does not swapping help? We can't build either operand as internal without swapping, so swapping cannot make things worse? > I agree that adding another small "do stmts match" check is a bit odd. > But I don't think making vect_build_slp_tree_1 return true here would > be good either, > and I haven't found a better way. > > Maybe need a wider refactoring of the swap heuristic. > Do that "stmts match" in the initial swap decision instead of relying > on later retry. > > > -- Richard Biener <[email protected]> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Jochen Jaser, Andrew McDonald; (HRB 36809, AG Nuernberg)
