On Tue, 28 Oct 2025, Christopher Bazley wrote:
> The vect_build_slp_instance function has a lot of parameters.
> Hitherto, it was undocumented whether it took ownership of the
> vectors named scalar_stmts, root_stmt_infos and remain.
>
> The calling code in vect_analyze_slp was written in such a way
> as to assume that vect_build_slp_instance always took ownership
> of the caller's reference to the passed-in scalar_stmts (i.e.
> that the vector need not be released by the caller on failure).
Actually it's written that vect_build_slp_tree takes ownership
on _successs_, but not on failure. But yes, the code is hard to
follow. All the wrappers, including vect_build_slp_instance
"deal with this", but it seems
if (bb_vec_info bb_vinfo = dyn_cast <bb_vec_info> (vinfo))
{
for (unsigned i = 0; i < bb_vinfo->roots.length (); ++i)
{
vect_location = bb_vinfo->roots[i].roots[0]->stmt;
/* Apply patterns. */
for (unsigned j = 0; j < bb_vinfo->roots[i].stmts.length ();
++j)
bb_vinfo->roots[i].stmts[j]
= vect_stmt_to_vectorize (bb_vinfo->roots[i].stmts[j]);
if (vect_build_slp_instance (bb_vinfo, bb_vinfo->roots[i].kind,
bb_vinfo->roots[i].stmts,
bb_vinfo->roots[i].roots,
bb_vinfo->roots[i].remain,
max_tree_size, &limit, bst_map,
false))
{
bb_vinfo->roots[i].stmts = vNULL;
bb_vinfo->roots[i].roots = vNULL;
bb_vinfo->roots[i].remain = vNULL;
}
}
is broken in that it does not honor vect_build_slp_instance
always taking ownership (but of scalar_stmts only)?
That is, I see there's some issues here, but the
proposed fix does not look correct (see below).
I was hoping auto_vec and std::move and C++ magic could maybe
clean this ownership mess up a bit, but my patience ran out
too fast (a few weeks ago).
> That contract might have been alright in principle (albeit
> inconsistent with the treatment of root_stmt_infos) except that
> vect_build_slp_tree can succeed when called within
> vect_build_slp_instance before vect_build_slp_instance fails.
>
> vect_build_slp_tree puts a reference to the stmts into the
> bst_map, but does not undo that on the failure recovery path.
> Consequently, the destructor for the _bb_vec_info later
> crashed upon trying to release the stmts of one of its roots
> because the stmts had already been released by the destructor
> of the _slp_tree created by vect_build_slp_tree (when
> invoked via release_scalar_stmts_to_slp_tree_map).
>
> There was also a potential leak in the case where *limit is 0
> (which causes false to be returned without releasing anything).
>
> Another broken failure recovery path occurred if
> vect_build_slp_instance detected that unrolling could have
> been required for BB SLP and the maximum number of vector
> elements for the tree was a known constant not greater than
> the group size. In this case, the tree node was freed *and*
> scalar_stmts was released (despite ownership having passed
> to the tree).
>
> Simplified the implementation of vect_build_slp_instance by
> not calling vect_build_slp_tree until after all other
> checks are completed. Simplified the contract with its
> callers by requiring them to release scalar_stmts on
> failure.
>
> The "Save for re-processing on failure" code was brittle because if
> the length of scalar_stmts was greater than one and
> vect_build_slp_instance failed then the length of scalar_stmts would
> have been queried again, despite ownership of scalar_stmts having
> already passed to vect_build_slp_instance. We now avoid querying the
> length a second time.
> ---
> gcc/tree-vect-slp.cc | 127 ++++++++++++++++++++++++-------------------
> 1 file changed, 72 insertions(+), 55 deletions(-)
>
> diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
> index 89bbd13fcb9..8b66751a8a9 100644
> --- a/gcc/tree-vect-slp.cc
> +++ b/gcc/tree-vect-slp.cc
> @@ -4062,7 +4062,9 @@ vect_build_slp_store_interleaving (vec<slp_tree>
> &rhs_nodes,
> }
>
> /* Analyze an SLP instance starting from SCALAR_STMTS which are a group
> - of KIND. Return true if successful. */
> + of KIND. Return true if successful. VEC_INFO and BST_MAP take joint
> + ownership of SCALAR_STMTS if successful. VEC_INFO takes ownership of
> + ROOT_STMT_INFOS and REMAIN if successful. */
>
> static bool
> vect_build_slp_instance (vec_info *vinfo,
> @@ -4115,10 +4117,6 @@ vect_build_slp_instance (vec_info *vinfo,
> matches[1] = false;
> }
> else
> - node = vect_build_slp_tree (vinfo, scalar_stmts, group_size,
> - &nunits, matches, limit,
> - &tree_size, bst_map);
> - if (node != NULL)
> {
> /* Calculate the unrolling factor based on the smallest type. */
> poly_uint64 unrolling_factor
Here we use max_nunits and that's set by vect_build_slp_tree so I don't
see how you can delay that.
> @@ -4136,7 +4134,6 @@ vect_build_slp_instance (vec_info *vinfo,
> "Build SLP failed: store group "
> "size not a multiple of the vector size "
> "in basic block SLP\n");
> - vect_free_slp_tree (node);
> return false;
> }
> /* Fatal mismatch. */
> @@ -4146,46 +4143,52 @@ vect_build_slp_instance (vec_info *vinfo,
> "splitting\n");
> memset (matches, true, group_size);
> matches[group_size / const_max_nunits * const_max_nunits] = false;
> - vect_free_slp_tree (node);
> }
> - else
> + else
> {
> - /* Create a new SLP instance. */
> - slp_instance new_instance = XNEW (class _slp_instance);
> - SLP_INSTANCE_TREE (new_instance) = node;
> - SLP_INSTANCE_LOADS (new_instance) = vNULL;
> - SLP_INSTANCE_ROOT_STMTS (new_instance) = root_stmt_infos;
> - SLP_INSTANCE_REMAIN_DEFS (new_instance) = remain;
> - SLP_INSTANCE_KIND (new_instance) = kind;
> - new_instance->reduc_phis = NULL;
> - new_instance->cost_vec = vNULL;
> - new_instance->subgraph_entries = vNULL;
> + node = vect_build_slp_tree (vinfo, scalar_stmts, group_size, &nunits,
> + matches, limit, &tree_size, bst_map);
> + }
> + }
>
> - if (dump_enabled_p ())
> - dump_printf_loc (MSG_NOTE, vect_location,
> - "SLP size %u vs. limit %u.\n",
> - tree_size, max_tree_size);
> + if (node != NULL)
> + {
> + /* Create a new SLP instance. */
> + slp_instance new_instance = XNEW (class _slp_instance);
> + SLP_INSTANCE_TREE (new_instance) = node;
> + SLP_INSTANCE_LOADS (new_instance) = vNULL;
> + SLP_INSTANCE_ROOT_STMTS (new_instance) = root_stmt_infos;
> + SLP_INSTANCE_REMAIN_DEFS (new_instance) = remain;
> + SLP_INSTANCE_KIND (new_instance) = kind;
> + new_instance->reduc_phis = NULL;
> + new_instance->cost_vec = vNULL;
> + new_instance->subgraph_entries = vNULL;
>
> - vinfo->slp_instances.safe_push (new_instance);
> + if (dump_enabled_p ())
> + dump_printf_loc (MSG_NOTE, vect_location, "SLP size %u vs limit %u.\n",
> + tree_size, max_tree_size);
>
> - /* ??? 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);
> + vinfo->slp_instances.safe_push (new_instance);
>
> - if (dump_enabled_p ())
> - {
> - dump_printf_loc (MSG_NOTE, vect_location,
> - "Final SLP tree for instance %p:\n",
> - (void *) new_instance);
> - vect_print_slp_graph (MSG_NOTE, vect_location,
> - SLP_INSTANCE_TREE (new_instance));
> - }
> + /* ??? 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);
>
> - return true;
> + if (dump_enabled_p ())
> + {
> + dump_printf_loc (MSG_NOTE, vect_location,
> + "Final SLP tree for instance %p:\n",
> + (void *) new_instance);
> + vect_print_slp_graph (MSG_NOTE, vect_location,
> + SLP_INSTANCE_TREE (new_instance));
> }
> +
> + return true;
> }
> +
> /* Failed to SLP. */
>
> /* While we arrive here even with slp_inst_kind_store we should only
> @@ -4193,9 +4196,6 @@ vect_build_slp_instance (vec_info *vinfo,
> vect_analyze_slp_instance now. */
> gcc_assert (kind != slp_inst_kind_store || group_size == 1);
>
> - /* Free the allocated memory. */
> - scalar_stmts.release ();
> -
> /* Failed to SLP. */
> if (dump_enabled_p ())
> dump_printf_loc (MSG_NOTE, vect_location, "SLP discovery failed\n");
> @@ -4699,7 +4699,6 @@ vect_analyze_slp_reduction (loop_vec_info vinfo,
> return true;
> }
> /* Failed to SLP. */
> -
> /* Free the allocated memory. */
> scalar_stmts.release ();
>
> @@ -5560,8 +5559,11 @@ vect_analyze_slp (vec_info *vinfo, unsigned
> max_tree_size,
> if (! vect_build_slp_instance (vinfo, slp_inst_kind_store,
> stmts, roots, remain, max_tree_size,
> &limit, bst_map, force_single_lane))
> - return opt_result::failure_at (vect_location,
> - "SLP build failed.\n");
> + {
> + stmts.release ();
> + return opt_result::failure_at (vect_location,
> + "SLP build failed.\n");
> + }
> }
>
> stmt_vec_info stmt_info;
> @@ -5575,8 +5577,11 @@ vect_analyze_slp (vec_info *vinfo, unsigned
> max_tree_size,
> if (! vect_build_slp_instance (vinfo, slp_inst_kind_store,
> stmts, roots, remain, max_tree_size,
> &limit, bst_map, force_single_lane))
> - return opt_result::failure_at (vect_location,
> - "SLP build failed.\n");
> + {
> + stmts.release ();
> + return opt_result::failure_at (vect_location,
> + "SLP build failed.\n");
> + }
> }
> }
>
> @@ -5634,8 +5639,11 @@ vect_analyze_slp (vec_info *vinfo, unsigned
> max_tree_size,
> max_tree_size, &limit,
> bst_map,
> force_single_lane))
> - return opt_result::failure_at (vect_location,
> - "SLP build failed.\n");
> + {
> + scalar_stmts.release ();
> + return opt_result::failure_at (vect_location,
> + "SLP build failed.\n");
> + }
> }
> }
> /* Save for re-processing on failure. */
> @@ -5649,8 +5657,7 @@ vect_analyze_slp (vec_info *vinfo, unsigned
> max_tree_size,
> max_tree_size, &limit, bst_map,
> force_single_lane))
> {
> - if (scalar_stmts.length () <= 1)
> - scalar_stmts.release ();
> + scalar_stmts.release ();
> /* Do SLP discovery for single-lane reductions. */
> for (auto stmt_info : saved_stmts)
> if (! vect_analyze_slp_reduction (loop_vinfo,
> @@ -5691,8 +5698,11 @@ vect_analyze_slp (vec_info *vinfo, unsigned
> max_tree_size,
> stmts, roots, remain,
> max_tree_size, &limit,
> bst_map, force_single_lane))
> - return opt_result::failure_at (vect_location,
> - "SLP build failed.\n");
> + {
> + stmts.release ();
> + return opt_result::failure_at (vect_location,
> + "SLP build failed.\n");
> + }
> }
> }
>
> @@ -5736,6 +5746,7 @@ vect_analyze_slp (vec_info *vinfo, unsigned
> max_tree_size,
> bst_map, force_single_lane))
> {
> roots.release ();
> + stmts.release ();
> return opt_result::failure_at (vect_location,
> "SLP build failed.\n");
> }
> @@ -5760,8 +5771,11 @@ vect_analyze_slp (vec_info *vinfo, unsigned
> max_tree_size,
> stmts, roots, remain,
> max_tree_size, &limit,
> bst_map, force_single_lane))
> - return opt_result::failure_at (vect_location,
> - "SLP build failed.\n");
> + {
> + stmts.release ();
> + return opt_result::failure_at (vect_location,
> + "SLP build failed.\n");
> + }
> }
> /* When the latch def is from a different cycle this can only
> be a induction. Build a simple instance for this.
> @@ -5778,8 +5792,11 @@ vect_analyze_slp (vec_info *vinfo, unsigned
> max_tree_size,
> stmts, roots, remain,
> max_tree_size, &limit,
> bst_map, force_single_lane))
> - return opt_result::failure_at (vect_location,
> - "SLP build failed.\n");
> + {
> + stmts.release ();
> + return opt_result::failure_at (vect_location,
> + "SLP build failed.\n");
> + }
> }
> }
> }
>
--
Richard Biener <[email protected]>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
