On Wed, 15 Jul 2026, Zhongyao Chen wrote:
> On Tue, Jul 14, 2026 at 7:36 PM Richard Biener <[email protected]> wrote:
> >
> > Sorry for the delay responding ...
>
> it's OK.
>
> > Hmm, so this probes ahead that we can likely successfully build
> > the swapped operand zero.
> >
> > Of course the first vect_build_slp_tree_1 is redundant (we'll
> > do it again during vect_build_slp_tree before swapping).
> >
> > So what I had in mind was sth like (just a quick sketch, not even
> > compile tested):
> >
> > diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
> > index ff11b392305..c1d82a8907e 100644
> > --- a/gcc/tree-vect-slp.cc
> > +++ b/gcc/tree-vect-slp.cc
> > @@ -3018,17 +3018,22 @@ out:
> > }
> > }
> >
> > - old_swap_distance = least_upthread_swappable_op_distance;
> > - if (can_swap_nonmatching)
> > - least_upthread_swappable_op_distance = 1;
> > - else if (least_upthread_swappable_op_distance != -1U)
> > - least_upthread_swappable_op_distance++;
> > child = vect_build_slp_tree (vinfo, oprnd_info->def_stmts,
> > &this_max_nunits,
> > matches, limit,
> > &this_tree_size, bst_map);
> > - least_upthread_swappable_op_distance = old_swap_distance;
> > - if (child != NULL)
> > +
> > + slp_tree saved_child = NULL;
> > + if (child
> > + && is_a <bb_vec_info> (vinfo)
> > + && SLP_TREE_DEF_TYPE (child) == vect_external_def
> > + && oprnd_info->first_dt != vect_external_def
> > + && oprnd_info->first_dt != vect_constant_def
> > + && can_swap_nonmatching)
> > + /* Check if matches[] from above is still populated correctly.
> > + I think it is. */
> > + saved_child = child;
> > + else if (child != NULL)
> > {
> > oprnd_info->def_stmts = vNULL;
> > children.safe_push (child);
> > @@ -3094,6 +3099,14 @@ out:
> > children.safe_push (child);
> > continue;
> > }
> > + if (saved_child)
> > + {
> > + /* Now un-swap stmts. matches[] should be still available
> > + from above. Important so we get operand 1 back. */
> > + oprnd_info->def_stmts = vNULL;
> > + children.safe_push (saved_child);
> > + continue;
> > + }
> > }
> > fail:
> >
>
> I tried your idea, with some changes.
>
> 1. The returned op0 child isn't itself external in PR125800:
>
> current commutative node
> `-- op0: internal child
> `-- external operand
>
> So the `SLP_TREE_DEF_TYPE (child) == vect_external_def` check doesn't
> catch it. I use the matches[] to detect the fallback and decide
> whether to save child and retry swap.
But isn't that good because we then pick the unswapped variant
immediately?
That was my idea to preserve the depth == 1 external build check at
least.
> 2. besides saved_child, I also save tree_size, max_nunits, and the
> swapped operand state, so a failed retry can be rolled back.
Yes, of course.
> all these changes make the code a little complicated and feel somewhat
> repetitive. do you see a cleaner way?
I suppose some bigger refactoring and C++ could help make it more
readable.
But as you also noticed the cache doesn't preserve matches[] as
it "fails" when we then build from scalars but it records all
success (from scalar build is success here), the whole idea
to re-use the first child build is probably fragile :/ That said,
the cache lookup leaves matches[] as-is (which is probably
less than ideal), that said - we could populate the ->failed[]
array explicitly when succeeding a from-scalar build and upon
cache hit restore that if available, plus document this use,
of course. I think that would make sense. And otherwise,
on cache-hit, make sure to fill matches[] with true.
Richard.
> Both PR125567 and PR125800 tests pass. haven't run other tests.
>
> Does this look OK?
>
>
>
> diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
> index 2250f6f74a1..f993589aa06 100644
> --- a/gcc/tree-vect-slp.cc
> +++ b/gcc/tree-vect-slp.cc
> @@ -1873,12 +1873,6 @@ vect_slp_linearize_chain (vec_info *vinfo,
> }
> }
>
> -/* Distance from the node currently being discovered to the closest upthread
> - commutative operation whose operand-zero discovery may still be fixed by
> - retrying with swapped operands, or -1U if there is none. */
> -
> -static unsigned least_upthread_swappable_op_distance = -1U;
> -
> static slp_tree
> vect_build_slp_tree_2 (vec_info *vinfo, slp_tree node,
> vec<stmt_vec_info> stmts, unsigned int group_size,
> @@ -2821,10 +2815,13 @@ out:
> {
> slp_tree child = nullptr;
> unsigned int j;
> - unsigned old_swap_distance;
> bool can_swap;
> bool can_swap_nonmatching;
> bool *stmt_can_swap;
> + slp_tree saved_child = NULL;
> + unsigned pre_child_tree_size;
> + poly_uint64 pre_child_max_nunits;
> + bool child_from_cache;
>
> /* We're skipping certain operands from processing, for example
> outer loop reduction initial defs. */
> @@ -3014,22 +3011,37 @@ out:
> can_swap_nonmatching = false;
> }
> }
> -
> - old_swap_distance = least_upthread_swappable_op_distance;
> - if (can_swap_nonmatching)
> - least_upthread_swappable_op_distance = 1;
> - else if (least_upthread_swappable_op_distance != -1U)
> - least_upthread_swappable_op_distance++;
> + pre_child_tree_size = this_tree_size;
> + pre_child_max_nunits = this_max_nunits;
> + /* A successful cache hit leaves matches unchanged. */
> + child_from_cache = bst_map->get (oprnd_info->def_stmts) != NULL;
> child = vect_build_slp_tree (vinfo, oprnd_info->def_stmts,
> group_size, &this_max_nunits,
> matches, limit,
> &this_tree_size, bst_map);
> - least_upthread_swappable_op_distance = old_swap_distance;
> +
> if (child != NULL)
> {
> - oprnd_info->def_stmts = vNULL;
> - children.safe_push (child);
> - continue;
> + if (is_a <bb_vec_info> (vinfo)
> + && !child_from_cache
> + && can_swap_nonmatching
> + && matches[0])
> + /* Use matches to detect an external operand built from scalars.
> + Save the child and retry later with swapped operands to see if
> + the build succeeds. */
> + for (j = 1; j < group_size; ++j)
> + if (!matches[j])
> + {
> + saved_child = child;
> + break;
> + }
> +
> + if (!saved_child)
> + {
> + oprnd_info->def_stmts = vNULL;
> + children.safe_push (child);
> + continue;
> + }
> }
>
> /* If the SLP build for operand zero failed and operand zero
> @@ -3060,6 +3072,17 @@ out:
> }
> while (j != group_size);
>
> + if (saved_child)
> + oprnd_info->def_stmts = oprnd_info->def_stmts.copy ();
> +
> + bool saved_any_pattern[2]
> + = { oprnds_info[0]->any_pattern,
> + oprnds_info[1]->any_pattern };
> + unsigned retry_tree_size
> + = saved_child ? pre_child_tree_size : this_tree_size;
> + poly_uint64 retry_max_nunits
> + = saved_child ? pre_child_max_nunits : this_max_nunits;
> +
> /* Swap mismatched definition stmts. */
> if (dump_enabled_p ())
> dump_printf_loc (MSG_NOTE, vect_location,
> @@ -3082,10 +3105,37 @@ out:
> oprnds_info[0]->any_pattern = oprnds_info[1]->any_pattern = true;
> /* And try again with scratch 'matches' ... */
> bool *tem = XALLOCAVEC (bool, group_size);
> - if ((child = vect_build_slp_tree (vinfo, oprnd_info->def_stmts,
> - group_size, &this_max_nunits,
> - tem, limit,
> - &this_tree_size, bst_map)) !=
> NULL)
> + child = vect_build_slp_tree (vinfo, oprnd_info->def_stmts,
> + group_size, &retry_max_nunits,
> + tem, limit,
> + &retry_tree_size, bst_map);
> + /* This is a speculative swap retry. Use child if it works;
> + otherwise restore the operand state and go back to saved_child.
> */
> + if (saved_child)
> + {
> + if (!child)
> + {
> + for (j = 0; j < group_size; ++j)
> + if (matches[j] == !swap_not_matching)
> + {
> + std::swap (oprnds_info[0]->def_stmts[j],
> + oprnds_info[1]->def_stmts[j]);
> + std::swap (oprnds_info[0]->ops[j],
> + oprnds_info[1]->ops[j]);
> + }
> + oprnds_info[0]->any_pattern = saved_any_pattern[0];
> + oprnds_info[1]->any_pattern = saved_any_pattern[1];
> + oprnd_info->def_stmts.release ();
> + children.safe_push (saved_child);
> + continue;
> + }
> +
> + vect_free_slp_tree (saved_child);
> + }
> +
> + this_tree_size = retry_tree_size;
> + this_max_nunits = retry_max_nunits;
> + if (child)
> {
> oprnd_info->def_stmts = vNULL;
> children.safe_push (child);
> @@ -3106,12 +3156,6 @@ fail:
> /* ??? Rejecting patterns this way doesn't work. We'd have to
> do extra work to cancel the pattern so the uses see the
> scalar version. */
> - /* Skip building vector operands from scalars while operand
> - discovery may still be fixed by retrying with swapped operands.
> */
> - && (least_upthread_swappable_op_distance != 1
> - /* A first scalar stmt mismatch signals a fatal mismatch
> - that the parent commutative retry cannot recover. */
> - || !matches[0])
> && !is_pattern_stmt_p (stmt_info)
> && !oprnd_info->any_pattern)
> {
>
>
>
> --
> Regards,
> Zhongyao
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Jochen Jaser, Andrew McDonald, Abhinav Puri; (HRB 36809, AG Nuernberg)
