On Wed, 8 Jul 2026, Tamar Christina wrote:
> > -----Original Message-----
> > From: Richard Biener <[email protected]>
> > Sent: 08 July 2026 12:29
> > To: Tamar Christina <[email protected]>
> > Cc: [email protected]; nd <[email protected]>; [email protected]
> > Subject: RE: [patch v2 2/2][vect]: replace usages of SLP_TREE_SCALAR_SMTS
> > where possible
> >
> > On Wed, 8 Jul 2026, Tamar Christina wrote:
> >
> > > > -----Original Message-----
> > > > From: Richard Biener <[email protected]>
> > > > Sent: 07 July 2026 12:26
> > > > To: Tamar Christina <[email protected]>
> > > > Cc: [email protected]; nd <[email protected]>; [email protected]
> > > > Subject: Re: [patch v2 2/2][vect]: replace usages of
> > SLP_TREE_SCALAR_SMTS
> > > > where possible
> > > >
> > > > On Mon, 6 Jul 2026, Tamar Christina wrote:
> > > >
> > > > > Continuing the work started in GCC 12 with g:a0dae768c6f78eba
> > > > > this patch replaces uses of SLP_TREE_SCALAR_STMTS with
> > SLP_TREE_LANES
> > > > when used
> > > > > to determine the number of lanes in the SLP tree. This de-couples the
> > need
> > > > to
> > > > > have the same number of scalar statements as lanes in the SLP tree.
> > > > >
> > > > > Bootstrapped Regtested on aarch64-none-linux-gnu,
> > > > > arm-none-linux-gnueabihf, x86_64-pc-linux-gnu
> > > > > -m32, -m64 and no issues.
> > > > >
> > > > > Any feedback?
> > > > >
> > > > > Thanks,
> > > > > Tamar
> > > > >
> > > > > gcc/ChangeLog:
> > > > >
> > > > > * tree-vect-slp-patterns.cc (compatible_complex_nodes_p):
> > > > > Support
> > > > > mismatched group sizes and use group size to iterate.
> > > > > * tree-vect-slp.cc (vect_build_slp_instance,
> > > > > vect_analyze_slp_reduc_chain, vect_analyze_slp_reduction,
> > > > > vect_analyze_slp_reduction_group, vect_analyze_slp_instance,
> > > > > vect_analyze_slp): Compare against lanes instead of .length ().
> > > > > (vect_optimize_slp_pass::get_result_with_layout): replace
> > > > > .length ()
> > > > > with .exist ().
> > > > > (vect_transform_slp_perm_load_1): Use lanes instead of .length
> > > > > ().
> > > > > (vect_remove_slp_scalar_calls): Replace iterators.
> > > > > * tree-vect-stmts.cc (vectorizable_shift): Use group size over
> > > > > ops
> > > > > length.
> > > > >
> > > > > ---
> > > > > diff --git a/gcc/tree-vect-slp-patterns.cc
> > > > > b/gcc/tree-vect-slp-patterns.cc
> > > > > index
> > > >
> > 181281c2154bedfc8e8c6b1bde1226e194ba99e5..3194642ef88e20b60ead
> > > > c44b1d69c0d4f5a59a96 100644
> > > > > --- a/gcc/tree-vect-slp-patterns.cc
> > > > > +++ b/gcc/tree-vect-slp-patterns.cc
> > > > > @@ -762,7 +762,10 @@ compatible_complex_nodes_p
> > > > (slp_compat_nodes_map_t *compat_cache,
> > > > > are externals. */
> > > > > if (SLP_TREE_DEF_TYPE (a) != vect_internal_def)
> > > > > {
> > > > > - for (unsigned i = 0; i < SLP_TREE_SCALAR_OPS (a).length ();
> > > > > i++)
> > > > > + unsigned group_size = SLP_TREE_LANES (a);
> > > > > + gcc_assert (SLP_TREE_SCALAR_OPS (a).length () == group_size
> > > > > + && SLP_TREE_SCALAR_OPS (b).length () == group_size);
> > > > > + for (unsigned i = 0; i < group_size; i++)
> > > >
> > > > OK.
> > > >
> > > > > {
> > > > > tree op1 = SLP_TREE_SCALAR_OPS (a)[pa[i % 2]];
> > > > > tree op2 = SLP_TREE_SCALAR_OPS (b)[pb[i % 2]];
> > > > > diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
> > > > > index
> > > >
> > 514bc0c5c654cc392ab5a47ea6b08b65fe7786fe..e74556b7dc558f931482b
> > > > 9dc1300d90c84b008fd 100644
> > > > > --- a/gcc/tree-vect-slp.cc
> > > > > +++ b/gcc/tree-vect-slp.cc
> > > > > @@ -4285,8 +4285,8 @@ vect_build_slp_instance (vec_info *vinfo,
> > > > > /* ??? We've replaced the old SLP_INSTANCE_GROUP_SIZE with
> > > > > the number of scalar stmts in the root in a few places.
> > > > > Verify that assumption holds. */
> > > >
> > > > The comment and the copies need adjustment with the change. I wonder
> > > > whether at the point this was added SLP_TREE_LANES existed?
> > > >
> > > > That said, group_size in this function is the number of "scalar"
> > > > stmts. I think we want to elide 'group_size' from the
> > > > vect_build_slp_tree API since this is not a "global" invariant
> > > > but the number of lanes could change throughout the SLP graph.
> > > >
> > >
> > > Yeah, I guess something like a narrowing of a pair of vectors and
> > > combining
> > > them changes the 'group_size'.
> >
> > Yes, or like asked for in PR126099, or PR126080.
>
> Thanks, hadn't seen those.
>
> >
> > > > The assertions are still somewhat relevant in that the root nodes
> > > > number of lanes should match the number of lanes from the scalar
> > > > stmt seeds.
> > >
> > > Hmm is that always the case? Wouldn't in a 2 V4SI -> V8HI operation the
> > lanes
> > > between the seed and root be possibly mismatched?
> >
> > SLP discovery is required to reproduce the seed lanes so I can't see how.
> >
>
> Sure, I was thinking of something like
>
> { (cast)a, (cast)b }
>
> Where the seed is the constructor and cast is a truncating expression and b
> e.g. V4SI.
>
> > > And we also have ldst where usually the lanes in the root doesn't match
> > > the
> > > children.
> >
> > Note the "root" is the entry to the graph, not the leafs.
>
> Sure, I was referring to e.g. https://godbolt.org/z/W8jTv8qWx
>
> Where the seed has two lanes, or at least two scalar statements
>
> note: Final SLP tree for instance 0x17311410:
> note: node 0x1736c438 (max_nunits=4, refcnt=1) vector(4) float
> note: op template: *_12 = _13;
> note: stmt 0 *_12 = _13;
> note: stmt 1 *_19 = _20;
> note: children 0x1736bd08 0x1736c158 (store-lanes)
> note: node 0x1736bd08 (max_nunits=4, refcnt=3) vector(4) float
> note: op template: _13 = _4 + _11;
> note: stmt 0 _13 = _4 + _11;
> note: children 0x1736bdc0 0x1736be78
> note: node 0x1736bdc0 (max_nunits=4, refcnt=2) vector(4) float
> note: op template: _4 = *_3;
> note: stmt 0 _4 = *_3;
> note: load permutation { 0 }
> note: node 0x1736be78 (max_nunits=4, refcnt=2) vector(4) float
> note: op template: _11 = _6 * _10;
> note: stmt 0 _11 = _6 * _10;
> note: children 0x1736bf30 0x1736bfe8
> note: node 0x1736bf30 (max_nunits=4, refcnt=3) vector(4) float
> note: op template: _6 = *_5;
> note: stmt 0 _6 = *_5;
> note: load permutation { 0 }
> note: node 0x1736bfe8 (max_nunits=4, refcnt=2) vector(4) float
> note: op template: _10 = *_9;
> note: stmt 0 _10 = *_9;
> note: load permutation { 1 }
> note: node 0x1736c158 (max_nunits=4, refcnt=3) vector(4) float
> note: op template: _20 = _15 + _18;
> note: stmt 0 _20 = _15 + _18;
> note: children 0x1736c210 0x1736c2c8
> note: node 0x1736c210 (max_nunits=4, refcnt=2) vector(4) float
> note: op template: _15 = *_14;
> note: stmt 0 _15 = *_14;
> note: load permutation { 1 }
> note: node 0x1736c2c8 (max_nunits=4, refcnt=2) vector(4) float
> note: op template: _18 = _6 * _17;
> note: stmt 0 _18 = _6 * _17;
> note: children 0x1736bf30 0x1736c380
> note: node 0x1736c380 (max_nunits=4, refcnt=2) vector(4) float
> note: op template: _17 = *_16;
> note: stmt 0 _17 = *_16;
> note: load permutation { 0 }
>
> but this may be a case where the PP is now showing accurately.
> when matching them they were a bit awkward....
The above is from permute lowering, the store (root) still has
two lanes but with the iffy representation of store-lanes
(I have elided a VEC_PERM node that concats/interleaves here IIRC).
> Just to avoid confusion, It wasn't clear to me whether you wanted me
> to change the current patch for this aside from the comments update?
I think the patch is fine, it doesn't improve things though, and the
comment needs updating.
Richard.
> Thanks,
> Tamar
>
> >
> > > >
> > > > > - gcc_assert (SLP_TREE_SCALAR_STMTS (SLP_INSTANCE_TREE
> > > > (new_instance))
> > > > > - .length () == group_size);
> > > > > + gcc_assert (SLP_TREE_LANES (SLP_INSTANCE_TREE (new_instance))
> > > > > + == group_size);
> > > > >
> > > > > if (dump_enabled_p ())
> > > > > {
> > > > > @@ -4720,8 +4720,8 @@ vect_analyze_slp_reduc_chain (loop_vec_info
> > > > vinfo,
> > > > > /* ??? We've replaced the old SLP_INSTANCE_GROUP_SIZE with
> > > > > the number of scalar stmts in the root in a few places.
> > > > > Verify that assumption holds. */
> > > > > - gcc_assert (SLP_TREE_SCALAR_STMTS (SLP_INSTANCE_TREE
> > > > (new_instance))
> > > > > - .length () == group_size);
> > > > > + gcc_assert (SLP_TREE_LANES (SLP_INSTANCE_TREE (new_instance))
> > > > > + == group_size);
> > > > >
> > > > > if (dump_enabled_p ())
> > > > > {
> > > > > @@ -4809,8 +4809,8 @@ vect_analyze_slp_reduction (loop_vec_info
> > vinfo,
> > > > > /* ??? We've replaced the old SLP_INSTANCE_GROUP_SIZE with
> > > > > the number of scalar stmts in the root in a few places.
> > > > > Verify that assumption holds. */
> > > > > - gcc_assert (SLP_TREE_SCALAR_STMTS (SLP_INSTANCE_TREE
> > > > (new_instance))
> > > > > - .length () == group_size);
> > > > > + gcc_assert (SLP_TREE_LANES (SLP_INSTANCE_TREE (new_instance))
> > > > > + == group_size);
> > > > >
> > > > > if (dump_enabled_p ())
> > > > > {
> > > > > @@ -4882,8 +4882,8 @@ vect_analyze_slp_reduction_group
> > > > (loop_vec_info loop_vinfo,
> > > > > /* ??? We've replaced the old SLP_INSTANCE_GROUP_SIZE with
> > > > > the number of scalar stmts in the root in a few places.
> > > > > Verify that assumption holds. */
> > > > > - gcc_assert (SLP_TREE_SCALAR_STMTS (SLP_INSTANCE_TREE
> > > > (new_instance))
> > > > > - .length () == group_size);
> > > > > + gcc_assert (SLP_TREE_LANES (SLP_INSTANCE_TREE (new_instance))
> > > > > + == group_size);
> > > > >
> > > > > if (dump_enabled_p ())
> > > > > {
> > > > > @@ -5138,8 +5138,8 @@ vect_analyze_slp_instance (vec_info *vinfo,
> > > > > /* ??? We've replaced the old SLP_INSTANCE_GROUP_SIZE with
> > > > > the number of scalar stmts in the root in a few places.
> > > > > Verify that assumption holds. */
> > > > > - gcc_assert (SLP_TREE_SCALAR_STMTS (SLP_INSTANCE_TREE
> > > > (new_instance))
> > > > > - .length () == group_size);
> > > > > + gcc_assert (SLP_TREE_LANES (SLP_INSTANCE_TREE (new_instance))
> > > > > + == group_size);
> > > > >
> > > > > if (dump_enabled_p ())
> > > > > {
> > > > > @@ -5385,8 +5385,8 @@ vect_analyze_slp_instance (vec_info *vinfo,
> > > > > /* ??? We've replaced the old SLP_INSTANCE_GROUP_SIZE with
> > > > > the number of scalar stmts in the root in a few places.
> > > > > Verify that assumption holds. */
> > > > > - gcc_assert (SLP_TREE_SCALAR_STMTS (SLP_INSTANCE_TREE
> > > > (new_instance))
> > > > > - .length () == group_size);
> > > > > + gcc_assert (SLP_TREE_LANES (SLP_INSTANCE_TREE (new_instance))
> > > > > + == group_size);
> > > > >
> > > > > if (dump_enabled_p ())
> > > > > {
> > > > > @@ -7906,7 +7906,7 @@
> > vect_optimize_slp_pass::get_result_with_layout
> > > > (slp_tree node,
> > > > >
> > > > > unsigned int num_lanes = SLP_TREE_LANES (node);
> > > > > result = vect_create_new_slp_node (num_inputs, VEC_PERM_EXPR);
> > > > > - if (SLP_TREE_SCALAR_STMTS (node).length ())
> > > > > + if (SLP_TREE_SCALAR_STMTS (node).exists ())
> > > >
> > > > OK.
> > > >
> > > > > {
> > > > > auto &stmts = SLP_TREE_SCALAR_STMTS (result);
> > > > > stmts.safe_splice (SLP_TREE_SCALAR_STMTS (node));
> > > > > @@ -8819,7 +8819,7 @@ vect_prologue_cost_for_slp (vec_info *vinfo,
> > > > slp_tree node,
> > > > > constants can be implemented as load from the constant pool.
> > > > > When all elements are the same we can use a splat. */
> > > > > tree vectype = SLP_TREE_VECTYPE (node);
> > > > > - unsigned group_size = SLP_TREE_SCALAR_OPS (node).length ();
> > > > > + unsigned group_size = SLP_TREE_LANES (node);
> > > >
> > > > OK.
> > > >
> > > > > unsigned HOST_WIDE_INT const_nunits;
> > > > > unsigned nelt_limit;
> > > > > unsigned nvectors = vect_get_num_copies (vinfo, node);
> > > > > @@ -10987,7 +10987,7 @@ vect_transform_slp_perm_load_1
> > (vec_info
> > > > *vinfo, slp_tree node,
> > > > > stmt_vec_info stmt_info = SLP_TREE_SCALAR_STMTS (node)[0];
> > > > > int vec_index = 0;
> > > > > tree vectype = SLP_TREE_VECTYPE (node);
> > > > > - unsigned int group_size = SLP_TREE_SCALAR_STMTS (node).length ();
> > > > > + unsigned int group_size = SLP_TREE_LANES (node);
> > > >
> > > > OK.
> > > >
> > > > > unsigned int mask_element;
> > > > > unsigned dr_group_size;
> > > > > machine_mode mode;
> > > > > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
> > > > > index
> > > >
> > 76e24b2c1699b022e4480fc4aac87ff0b6ddf650..f41d49541d570b656eec1
> > > > 0227f7f030051acd0bb 100644
> > > > > --- a/gcc/tree-vect-stmts.cc
> > > > > +++ b/gcc/tree-vect-stmts.cc
> > > > > @@ -6388,15 +6388,18 @@ vectorizable_shift (vec_info *vinfo,
> > > > > /* Now adjust the constant shift amount in place. */
> > > > > if (incompatible_op1_vectype_p
> > > > > && dt[1] == vect_constant_def)
> > > > > - for (unsigned i = 0;
> > > > > - i < SLP_TREE_SCALAR_OPS (slp_op1).length (); ++i)
> > > > > - {
> > > > > - SLP_TREE_SCALAR_OPS (slp_op1)[i]
> > > > > - = fold_convert (TREE_TYPE (vectype),
> > > > > - SLP_TREE_SCALAR_OPS (slp_op1)[i]);
> > > > > - gcc_assert ((TREE_CODE (SLP_TREE_SCALAR_OPS (slp_op1)[i])
> > > > > - == INTEGER_CST));
> > > > > - }
> > > > > + {
> > > > > + unsigned group_size = SLP_TREE_LANES (slp_op1);
> > > > > + gcc_assert (SLP_TREE_SCALAR_OPS (slp_op1).length () ==
> > > > group_size);
> > > >
> > > > This is OK in principle, but how do we handle this when re-vectorizing
> > > > vector by scalar shifts? Have scalar stmts of the vector operand
> > > > { v0, v0, v0, v0 } and the scalar shift { s, s, s, s }, so both
> > > > expanded to be TYPE_VECTOR_SUBPARTS times?
> > >
> > > Yeah, my thoughts were that for constant arguments or externals that the
> > > node should be flattened. The lanes would match up but of course not the
> > > number of scalar statements.
> > >
> > > This does make it slightly awkward to decompose the tree but otoh it
> > > should
> > > just work when vectorizing the arguments, whereas a weird nested
> > operation
> > > would make decompose easier but vectorization harder.
> > >
> > > Thanks,
> > > Tamar
> > > >
> > > > > + for (unsigned i = 0; i < group_size; ++i)
> > > > > + {
> > > > > + SLP_TREE_SCALAR_OPS (slp_op1)[i]
> > > > > + = fold_convert (TREE_TYPE (vectype),
> > > > > + SLP_TREE_SCALAR_OPS (slp_op1)[i]);
> > > > > + gcc_assert ((TREE_CODE (SLP_TREE_SCALAR_OPS (slp_op1)[i])
> > > > > + == INTEGER_CST));
> > > > > + }
> > > > > + }
> > > > > SLP_TREE_TYPE (slp_node) = shift_vec_info_type;
> > > > > DUMP_VECT_SCOPE ("vectorizable_shift");
> > > > > vect_model_simple_cost (vinfo, 1, slp_node, cost_vec);
> > > > >
> > > > >
> > > > >
> > > >
> > > > --
> > > > Richard Biener <[email protected]>
> > > > SUSE Software Solutions Germany GmbH,
> > > > Frankenstrasse 146, 90461 Nuernberg, Germany;
> > > > GF: Jochen Jaser, Andrew McDonald; (HRB 36809, AG Nuernberg)
> > >
> >
> > --
> > Richard Biener <[email protected]>
> > SUSE Software Solutions Germany GmbH,
> > Frankenstrasse 146, 90461 Nuernberg, Germany;
> > GF: Jochen Jaser, Andrew McDonald; (HRB 36809, AG Nuernberg)
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Jochen Jaser, Andrew McDonald; (HRB 36809, AG Nuernberg)
