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).
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
@@ -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");
+ }
}
}
}
--
2.43.0
