On 8/21/24 3:09 PM, Iain Sandoe wrote:
This is primarily preparation to partition the functionality of the
coroutine transform into analysis, ramp generation and then (later)
synthesis of the coroutine body. The patch does fix one latent
issue in the ordering of DTORs for frame parameter copies (to ensure
that they are processed in reverse order to the copy creation).
gcc/cp/ChangeLog:
* coroutines.cc (build_actor_fn): Arrange to apply any
required parameter copy DTORs in reverse order to their
creation.
(morph_fn_to_coro): Split the ramp function completion
into a separate function.
(build_ramp_function): New.
Signed-off-by: Iain Sandoe <i...@sandoe.co.uk>
---
gcc/cp/coroutines.cc | 360 +++++++++++++++++++++++--------------------
1 file changed, 192 insertions(+), 168 deletions(-)
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 1f1ea5c2fe4..50362fc3556 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
+build_ramp_function (tree orig, location_t fn_start, tree coro_frame_ptr,
+ tree coro_frame_type,
+ hash_map<tree, param_info> *param_uses,
+ tree act_des_fn_ptr, tree actor, tree destroy,
+ vec<tree> *param_dtor_list)
{
...
+ tree fn_return_type = TREE_TYPE (TREE_TYPE (orig));
/* Ramp: */
+ tree stmt = begin_function_body ();
The name "stmt" doesn't suggest to me that it's holding the result of
begin_function_body. Maybe "ramp_fnbody"? Of course, then there's some
confusion with "ramp_body". Should that also change?
+ /* Clean up any frame copies of parms with non-trivial dtors.
+ Do this in reverse order from their creation. */
+ vec<param_info *> worklist = vNULL;
I think this should be auto_vec, to avoid leaking the vec when the
function returns.
+bool
+morph_fn_to_coro (tree orig, tree *resumer, tree *destroyer)
+{
...
+ /* FIXME: This has the hidden side-effect of preparing the current function
+ to be the ramp. */
This comment seems obsolete?
+ tree fnbody = split_coroutine_body_from_ramp (orig);
+ if (!fnbody)
+ return false;
+
+ /* If the original function has a return value with a non-trivial DTOR
+ and the body contains a var with a DTOR that might throw, the decl is
+ marked "throwing_cleanup".
+ We do not [in the ramp, which is synthesised here], use any body var
+ types with DTORs that might throw.
+ The original body is transformed into the actor function which only
+ contains void returns, and is also wrapped in a try-catch block.
+ So (a) the 'throwing_cleanup' is not correct for the ramp and (b) we do
+ not need to transfer it to the actor which only contains void returns. */
+ cp_function_chain->throwing_cleanup = false;
Do we want to clear other members of cp_function_chain as well?
return_value, returns_value, returns_null, returns_abnormally,
infinite_loop, can_throw, invalid_constexpr, x_named_labels, bindings,
infinite_loops all seem disconnected from the spliced definition.
+ hash_map<tree, param_info> *param_uses = analyze_fn_parms (orig);
This map seems to leak. How about making it a local variable (like
suspend_points) and passing its address to analyze_fn_parms?
+ fnbody = coro_rewrite_function_body (fn_start, fnbody, orig, param_uses,
+ act_des_fn_ptr,
+ resume_idx_var, fs_label);
+
+ /* We need to know, and inspect, each suspend point in the function
+ in several places. It's convenient to place this map out of line
+ since it's used from tree walk callbacks. */
+
+ hash_map<tree, suspend_point_info> suspend_points;
+ /* Now insert the data for any body await points, at this time we also need
+ to promote any temporaries that are captured by reference (to regular
+ vars) they will get added to the coro frame along with other locals. */
+ susp_frame_data body_aw_points (fs_label, &suspend_points);
+ cp_walk_tree (&fnbody, await_statement_walker, &body_aw_points, NULL);
+
+ /* 4. Now make space for local vars, this is conservative again, and we
+ would expect to delete unused entries later. Compose the frame entries
+ list. */
Is this comment still accurate? It seems surprising given that you're
collecting uses.
+ /* The fields for the coro frame. */
+ tree field_list = NULL_TREE;
+ hash_map<tree, local_var_info> local_var_uses;
+ local_vars_frame_data local_vars_data (&field_list, &local_var_uses);
+ cp_walk_tree (&fnbody, register_local_var_uses, &local_vars_data, NULL);
+
...
+ vec<tree> param_dtor_list = vNULL;
This should also be auto_vec.
Jason
