Hi Richard,
thanks for the review. Comments below.
On 03/08/2026 11:56, Richard Biener wrote:
On Fri, Jul 17, 2026 at 3:32 PM Christopher Bazley via Sourceware
Forge <[email protected]> wrote:
From: Christopher Bazley <[email protected]>
vect_create_vectorized_promotion_stmts no longer pushes
more stmts than implied by vect_get_num_copies because it could
previously overrun the number of slots allocated for an SLP node
(based on its number of lanes and type). e.g., four defs were
pushed for a promotion of V8HI to V2DI (8/2=4) even if only two
lanes of the V8HI were active. Allowing it later caused ICE in
vectorizable_operation for a parent node, because binary ops
require both operands to be the same length.
Since promotion no longer produces redundant definitions,
vectorizable_conversion also had to be modified so that demotion no
longer relies on an even number of defs being produced. If
necessary, it now pushes a single constant zero def.
gcc/ChangeLog:
* tree-vect-stmts.cc (vect_create_vectorized_promotion_stmts):
Require an SLP tree node to be passed by the caller, for use
by vect_get_num_copies. Stop pushing more stmts than implied
by vect_get_num_copies.
(vectorizable_conversion): Pass SLP tree node to
vect_create_vectorized_promotion_stmts. Demotion no longer
relies on an even number of definitions being produced by
promotion. If necessary, push a single constant zero
definition.
---
gcc/tree-vect-stmts.cc | 61 ++++++++++++++++++++++++++----------------
1 file changed, 38 insertions(+), 23 deletions(-)
diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
index 700b3e7e0686..5ce9db48e286 100644
--- a/gcc/tree-vect-stmts.cc
+++ b/gcc/tree-vect-stmts.cc
@@ -5167,7 +5167,7 @@ vect_create_vectorized_demotion_stmts (vec_info *vinfo,
vec<tree> *vec_oprnds,
call the function recursively. */
static void
-vect_create_vectorized_promotion_stmts (vec_info *vinfo,
+vect_create_vectorized_promotion_stmts (vec_info *vinfo, slp_tree slp_node,
vec<tree> *vec_oprnds0,
vec<tree> *vec_oprnds1,
stmt_vec_info stmt_info, tree vec_dest,
@@ -5180,37 +5180,39 @@ vect_create_vectorized_promotion_stmts (vec_info *vinfo,
gimple *new_stmt1, *new_stmt2;
vec<tree> vec_tmp = vNULL;
- vec_tmp.create (vec_oprnds0->length () * 2);
+ const unsigned ncopies = vect_get_num_copies (vinfo, slp_node);
+ vec_tmp.create (ncopies);
+ gcc_assert (vec_oprnds0->length () <= ncopies);
FOR_EACH_VEC_ELT (*vec_oprnds0, i, vop0)
{
+ if (vec_tmp.length () >= ncopies)
+ break;
Can this ever happen? We'd have a completely unused vector operand.
I understand in your case we have the upper part of a vector unused.
I think this warrants a comment, possibly before the loop, to document
the input/output cases we support - formerly it was two times the number
of output vectors due to the "same vector size" constraint. I wonder
why you do not need to adjust the callers modifier (NONE/WIDEN/NARROW)
handling?
This change will no longer exist after rebasing my patch because
upstream now passes vect_create_vectorized_promotion_stmts an argument
that limits the number of outputs.
+
if (op_type == binary_op)
vop1 = (*vec_oprnds1)[i];
else
vop1 = NULL_TREE;
/* Generate the two halves of promotion operation. */
- new_stmt1 = vect_gen_widened_results_half (vinfo, ch1, vop0, vop1,
- op_type, vec_dest, gsi,
- stmt_info);
- new_stmt2 = vect_gen_widened_results_half (vinfo, ch2, vop0, vop1,
- op_type, vec_dest, gsi,
- stmt_info);
- if (is_gimple_call (new_stmt1))
- {
- new_tmp1 = gimple_call_lhs (new_stmt1);
- new_tmp2 = gimple_call_lhs (new_stmt2);
- }
- else
+ new_stmt1
+ = vect_gen_widened_results_half (vinfo, ch1, vop0, vop1, op_type,
+ vec_dest, gsi, stmt_info);
+ new_tmp1 = is_gimple_call (new_stmt1) ? gimple_call_lhs (new_stmt1)
+ : gimple_assign_lhs (new_stmt1);
gimple_get_lhs (new_stmt1);
This change will no longer exist after rebasing my patch.
+ vec_tmp.quick_push (new_tmp1);
+
+ if (vec_tmp.length () < ncopies)
{
- new_tmp1 = gimple_assign_lhs (new_stmt1);
- new_tmp2 = gimple_assign_lhs (new_stmt2);
+ new_stmt2
+ = vect_gen_widened_results_half (vinfo, ch2, vop0, vop1, op_type,
+ vec_dest, gsi, stmt_info);
+ new_tmp2 = is_gimple_call (new_stmt2) ? gimple_call_lhs (new_stmt2)
+ : gimple_assign_lhs (new_stmt2);
likewise.
This change will no longer exist after rebasing my patch.
+ vec_tmp.quick_push (new_tmp2);
}
-
- /* Store the results for the next step. */
- vec_tmp.quick_push (new_tmp1);
- vec_tmp.quick_push (new_tmp2);
}
+ gcc_assert (vec_tmp.length () <= ncopies);
No need for this, quick_push will ICE already.
This change will no longer exist after rebasing my patch.
vec_oprnds0->release ();
*vec_oprnds0 = vec_tmp;
}
@@ -5832,12 +5834,15 @@ vectorizable_conversion (vec_info *vinfo,
stmt_info, this_dest, gsi, c1,
op_type);
else
- vect_create_vectorized_promotion_stmts (vinfo, &vec_oprnds0,
- &vec_oprnds1, stmt_info,
- this_dest, gsi,
+ vect_create_vectorized_promotion_stmts (vinfo, slp_node,
+ &vec_oprnds0, &vec_oprnds1,
+ stmt_info, this_dest, gsi,
c1, c2, op_type);
}
+ gcc_assert (vec_oprnds0.length ()
+ == vect_get_num_copies (vinfo, slp_node));
No need for such asserts IMO.
OK. I will delete it.
+
FOR_EACH_VEC_ELT (vec_oprnds0, i, vop0)
{
gimple *new_stmt;
@@ -5861,6 +5866,16 @@ vectorizable_conversion (vec_info *vinfo,
generate more than one vector stmt - i.e - we need to "unroll"
the vector stmt by a factor VF/nunits. */
vect_get_vec_defs (vinfo, slp_node, op0, &vec_oprnds0);
+
+ /* Promotion no longer produces redundant defs (since support was
+ added for length/mask-predicated BB SLP of awkward-sized groups),
+ therefore demotion now has to handle that case too. */
+ if (vec_oprnds0.length () % 2 != 0)
+ {
+ tree vectype = TREE_TYPE (vec_oprnds0[0]);
+ vec_oprnds0.safe_push (build_zero_cst (vectype));
Ah, so it's for those zeros?
Yes. The zero supplies the missing member of the final pair consumed by
demotion. I will clarify this comment to say that explicitly.
+ }
+
/* Arguments are ready. Create the new vector stmts. */
if (cvt_type && modifier == NARROW_DST)
FOR_EACH_VEC_ELT (vec_oprnds0, i, vop0)
--
2.54.0
--
Christopher Bazley
Staff Software Engineer, GNU Tools Team.
Arm Ltd, 110 Fulbourn Road, Cambridge, CB1 9NJ, UK.
http://www.arm.com/
