On Fri, Jul 17, 2026 at 4:36 PM Richard Biener <[email protected]> wrote:
> > 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.
both tests have the same shape:
P: current commutative node
`-- A: op0: internal child
`-- E: external operand
PR125800 wants the unswapped variant, while PR125567 wants the swapped
one. So checking SLP_TREE_DEF_TYPE (child) skip them both and keep
unswapped.
checking A's child doesn't work either, since it may be a normal
external input rather than one created by the failback path.
> > 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.
>
Right. Here is my new try.
diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
index 2250f6f74a1..132930e9af7 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,
@@ -1886,6 +1880,30 @@ vect_build_slp_tree_2 (vec_info *vinfo, slp_tree node,
bool *matches, unsigned *limit, unsigned *tree_size,
scalar_stmts_to_slp_tree_map_t *bst_map);
+/* Restore fallback matches for NODE, or all true for a clean build. */
+
+static void
+vect_restore_slp_matches (slp_tree node, bool *matches, unsigned group_size)
+{
+ if (node->failed)
+ memcpy (matches, node->failed, sizeof (bool) * group_size);
+ else
+ for (unsigned i = 0; i < group_size; ++i)
+ matches[i] = true;
+}
+
+/* Record the fallback and first retryable MATCHES, if any. */
+
+static void
+vect_record_slp_fallback_matches (slp_tree node, bool *matches,
+ unsigned group_size)
+{
+ if (!node->failed)
+ node->failed = XCNEWVEC (bool, group_size);
+ if (matches[0] && !node->failed[0])
+ memcpy (node->failed, matches, sizeof (bool) * group_size);
+}
+
static slp_tree
vect_build_slp_tree (vec_info *vinfo,
vec<stmt_vec_info> stmts, unsigned int group_size,
@@ -1895,17 +1913,23 @@ vect_build_slp_tree (vec_info *vinfo,
{
if (slp_tree *leader = bst_map->get (stmts))
{
+ /* failed also stores matches for a successful scalar fallback, so use
+ def_type to tell whether the build failed. */
+ bool failed_p
+ = SLP_TREE_DEF_TYPE (*leader) == vect_uninitialized_def;
if (dump_enabled_p ())
dump_printf_loc (MSG_NOTE, vect_location, "re-using %sSLP tree %p\n",
- !(*leader)->failed ? "" : "failed ",
+ failed_p ? "failed " : "",
(void *) *leader);
- if (!(*leader)->failed)
+ if (!failed_p)
{
+ vect_restore_slp_matches (*leader, matches, group_size);
SLP_TREE_REF_COUNT (*leader)++;
vect_update_max_nunits (max_nunits, (*leader)->max_nunits);
stmts.release ();
return *leader;
}
+ gcc_assert ((*leader)->failed);
memcpy (matches, (*leader)->failed, sizeof (bool) * group_size);
return NULL;
}
@@ -1949,7 +1973,8 @@ vect_build_slp_tree (vec_info *vinfo,
as backedge destinations. */
SLP_TREE_SCALAR_STMTS (res) = vNULL;
SLP_TREE_DEF_TYPE (res) = vect_uninitialized_def;
- res->failed = XNEWVEC (bool, group_size);
+ if (!res->failed)
+ res->failed = XNEWVEC (bool, group_size);
if (flag_checking)
{
unsigned i;
@@ -1969,6 +1994,7 @@ vect_build_slp_tree (vec_info *vinfo,
gcc_assert (res_ == res);
res->max_nunits = this_max_nunits;
vect_update_max_nunits (max_nunits, this_max_nunits);
+ vect_restore_slp_matches (res, matches, group_size);
/* Keep a reference for the bst_map use. */
SLP_TREE_REF_COUNT (res)++;
}
@@ -2821,10 +2847,12 @@ 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;
/* We're skipping certain operands from processing, for example
outer loop reduction initial defs. */
@@ -3014,22 +3042,31 @@ 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;
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)
+ && can_swap_nonmatching
+ && matches[0])
+ 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 +3097,18 @@ out:
}
while (j != group_size);
+ if (saved_child)
+ oprnd_info->def_stmts
+ = SLP_TREE_SCALAR_STMTS (saved_child).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 +3131,45 @@ 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 only for a clean
+ success; otherwise restore the operand state and saved_child. */
+ if (saved_child)
+ {
+ if (!child || child->failed)
+ {
+ /* A successful build owns the retry stmts, or released them
+ on a cache hit. Copy them back before undoing
the swap. */
+ if (child)
+ {
+ oprnd_info->def_stmts
+ = SLP_TREE_SCALAR_STMTS (child).copy ();
+ vect_free_slp_tree (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 +3190,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)
{
@@ -3129,6 +3207,7 @@ fail:
if (dump_enabled_p ())
dump_printf_loc (MSG_NOTE, vect_location,
"Building vector operands from scalars\n");
+ vect_record_slp_fallback_matches (node, matches, group_size);
this_tree_size++;
child = vect_create_new_slp_node (oprnd_info->ops);
children.safe_push (child);
diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
index 0b646d6b29e..a21ca7e3ba1 100644
--- a/gcc/tree-vectorizer.h
+++ b/gcc/tree-vectorizer.h
@@ -391,9 +391,9 @@ struct _slp_tree {
template <class T>
T& get_data (T& else_) { return data ? *static_cast <T *> (data) : else_; }
- /* If not NULL this is a cached failed SLP discovery attempt with
- the lanes that failed during SLP discovery as 'false'. This is
- a copy of the matches array. */
+ /* def_type tells whether the cached build succeeded. On success, failed
+ is NULL for a clean build and otherwise holds scalar-fallback matches.
+ On failure, failed holds the matches of the failed build. */
bool *failed;
/* Allocate from slp_tree_pool. */
--
Regards,
Zhongyao
