On Tue, 23 Jun 2026, Zhongyao Chen wrote:
> > I'm trying to understand from the above and the patch, but fail. What's
> > the set of stmts we avoid to externalize because we can swap the
> > (immediate) parent?
> >
> > As said in the PR for the initial patch for the greedy search we
> > perform any heuristic will have downsides and for BB vectorization
> > swapping is as good as not swapping if operand zero discovery
> > ends up with an externalized node.
> >
> > Thanks,
> > Richard.
>
> Hi Richard,
>
> In this regression case, operand-zero discovery does end up
> externalized either way. I'm not trying to avoid externalization
> itself here; what I'm trying to avoid is introducing additional
> parent-level externalization.
>
> In this case, the immediate-parent swap retry is harmful. It swaps
> lanes that are already aligned with the first-lane operand columns,
> breaks that local shape, and then the externalization propagates
> upward. So we do not just externalize the child, we externalize the
> parent as well, which gives a shorter SLP tree and worse codegen.
>
> Without this patch, the dump shows:
> ```
> Build SLP failed: different BIT_FIELD_REF arguments in
> stmp_isum10_93.49_256 = BIT_FIELD_REF <vect__30.48_255, 64, 0>;
> SLP discovery for node ... failed
> SLP discovery for node ... failed
> Re-trying with swapped operands of stmts 1 2 3
> Build SLP for stmp_rsum00_88.79_304 = rsum00_106 + stmp_rsum00_88.79_303;
> Build SLP for stmp_isum00_89.75_298 = BIT_FIELD_REF <vect__18.74_295, 64, 64>;
> Build SLP failed: different operation in stmt
> stmp_isum00_89.75_298 = BIT_FIELD_REF <vect__18.74_295, 64, 64>;
> original stmt
> stmp_rsum00_88.79_304 = rsum00_106 + stmp_rsum00_88.79_303;
> ...
> Building vector operands from scalars
> Building parent vector operands from scalars instead
> ...
> SLP size 15 vs. limit 176.
> ```
>
> With this patch, the child may still externalize, but we skip that
> harmful immediate-parent retry, so the larger parent stays internal.
> The dump then looks like:
>
> ```
> Build SLP failed: different BIT_FIELD_REF arguments in
> stmp_isum10_93.49_256 = BIT_FIELD_REF <vect__30.48_255, 64, 0>;
> SLP discovery for node ... failed
> Building vector operands from scalars
> SLP discovery for node ... succeeded
> ...
> Build SLP for rsum00_147 = _148 + rsum00_181;
> Build SLP for isum00_143 = _144 + isum00_180;
> Build SLP for rsum10_131 = _132 + rsum10_177;
> Build SLP for isum10_127 = _128 + isum10_176;
> ...
> SLP size 19 vs. limit 176.
> ```
>
> So for this PR, "it ends up externalized anyway" is not the same as
> "swapping is as good as not swapping". The old retry makes the
> externalization spread to the parent and leaves a worse final SLP tree.
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;
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Jochen Jaser, Andrew McDonald; (HRB 36809, AG Nuernberg)
