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.
> > 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.
> 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.
> >
> > > - 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)
