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.
2. besides saved_child, I also save tree_size, max_nunits, and the
swapped operand state, so a failed retry can be rolled back.
all these changes make the code a little complicated and feel somewhat
repetitive. do you see a cleaner way?
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
