> On 2 Jun 2025, at 16:33, Iain Sandoe <i...@sandoe.co.uk> wrote:
>
> Hi Jason
>
>> On 2 Jun 2025, at 16:20, Jason Merrill <ja...@redhat.com> wrote:
>>
>> On 5/31/25 3:19 PM, Iain Sandoe wrote:
>>> Some small cleanups found while working on other changes, tested
>>> on x86_64-darwin, OK for trunk?
>>> thanks
>>> Iain
>>> --- 8< ---
>>> We were incorrectly guarding all the frame cleanups on the
>>> basis of frame_needs_free (which is always set for the present
>>> code-gen since we have no allocation elision). The net result
>>> being that the (incorrect) code was behaving as expected.
>>> We built, but never used, a label for the frame destruction;
>>> in practice it is never triggered independently of the promise
>>> and argument copy destruction.
>>
>> Would it make sense to call the destructor for the frame type as a whole
>> rather than the promise and arguments separately?
>
> At present,
> - because the standard describes the frame in terms of allocation and
> deallocation rules (although we do build a record type for it)
> - because the frame copies of local vars need to be constructed and
> destructed as needed (i.e. in the same manner as on the stack)
> (and especially since we want to “fold” the use of storage as an
> optimisation on the TODO (again in the manner of the stack))
>
> there is neither frame constructor nor destructor;
It is specifically the case that the coroutine frame object is *not* a first
class type - much EWG debate in particular with the BGN NB folks.
We reserve the right to modify it in arbitrary ways to optimise - the folding
of the storage is just the first we have an implementation for;
- we could also delete members late in the middle end if they were determined
to be unused.
- we could also do the “HALO” thing and remove the allocation in favour of
stack if the coroutine can be proven not to escape the caller’s context.
> So that is not a short-term solution - and possibly non-trivial - thoughts?
> Iain
>
>>
>>> Finally there are a few instances where we had been building
>>> expressions manually rather than using higher-level APIs.
>>> gcc/cp/ChangeLog:
>>> * coroutines.cc (build_actor_fn): Remove an unused
>>> label, guard the frame deallocation correctly, use
>>> simpler APIs to build if and return statements.
>>> Signed-off-by: Iain Sandoe <i...@sandoe.co.uk>
>>> ---
>>> gcc/cp/coroutines.cc | 58 ++++++++++++++++----------------------------
>>> 1 file changed, 21 insertions(+), 37 deletions(-)
>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>>> index aa00a8a4e68..a2b6d53805a 100644
>>> --- a/gcc/cp/coroutines.cc
>>> +++ b/gcc/cp/coroutines.cc
>>> @@ -2557,8 +2557,8 @@ build_actor_fn (location_t loc, tree coro_frame_type,
>>> tree actor, tree fnbody,
>>> /* Finish the resume dispatcher. */
>>> finish_switch_stmt (dispatcher);
>>> - finish_else_clause (lsb_if);
>>> + finish_else_clause (lsb_if);
>>> finish_if_stmt (lsb_if);
>>> /* If we reach here then we've hit UB. */
>>> @@ -2597,69 +2597,53 @@ build_actor_fn (location_t loc, tree
>>> coro_frame_type, tree actor, tree fnbody,
>>> /* Add in our function body with the co_returns rewritten to final form.
>>> */
>>> add_stmt (fnbody);
>>> - /* now do the tail of the function. */
>>> + /* Now do the tail of the function; first cleanups. */
>>> r = build_stmt (loc, LABEL_EXPR, del_promise_label);
>>> add_stmt (r);
>>> - /* Destructors for the things we built explicitly. */
>>> + /* Destructors for the things we built explicitly.
>>> + promise... */
>>> if (tree c = cxx_maybe_build_cleanup (promise_proxy, tf_warning_or_error))
>>> - add_stmt (c);
>>> -
>>> - tree del_frame_label
>>> - = create_named_label_with_ctx (loc, "coro.delete.frame", actor);
>>> - r = build_stmt (loc, LABEL_EXPR, del_frame_label);
>>> - add_stmt (r);
>>> -
>>> - /* Here deallocate the frame (if we allocated it), which we will have at
>>> - present. */
>>> - tree fnf2_x
>>> - = coro_build_frame_access_expr (actor_frame, coro_frame_needs_free_id,
>>> - false, tf_warning_or_error);
>>> + finish_expr_stmt (c);
>>> - tree need_free_if = begin_if_stmt ();
>>> - fnf2_x = build1 (CONVERT_EXPR, integer_type_node, fnf2_x);
>>> - tree cmp = build2 (NE_EXPR, integer_type_node, fnf2_x,
>>> integer_zero_node);
>>> - finish_if_stmt_cond (cmp, need_free_if);
>>> + /* Argument copies ... */
>>> while (!param_dtor_list->is_empty ())
>>> {
>>> tree parm_id = param_dtor_list->pop ();
>>> tree a = coro_build_frame_access_expr (actor_frame, parm_id, false,
>>> tf_warning_or_error);
>>> if (tree dtor = cxx_maybe_build_cleanup (a, tf_warning_or_error))
>>> - add_stmt (dtor);
>>> + finish_expr_stmt (dtor);
>>> }
>>> + /* Here deallocate the frame (if we allocated it), which we will have at
>>> + present. */
>>> + tree fnf2_x
>>> + = coro_build_frame_access_expr (actor_frame, coro_frame_needs_free_id,
>>> + false, tf_warning_or_error);
>>> + tree need_free_if = begin_if_stmt ();
>>> + finish_if_stmt_cond (fnf2_x, need_free_if);
>>> +
>>> /* Build the frame DTOR. */
>>> tree del_coro_fr
>>> = build_coroutine_frame_delete_expr (actor_fp, frame_size,
>>> promise_type, loc);
>>> finish_expr_stmt (del_coro_fr);
>>> finish_then_clause (need_free_if);
>>> - tree scope = IF_SCOPE (need_free_if);
>>> - IF_SCOPE (need_free_if) = NULL;
>>> - r = do_poplevel (scope);
>>> - add_stmt (r);
>>> + finish_if_stmt (need_free_if);
>>> - /* done. */
>>> - r = build_stmt (loc, RETURN_EXPR, NULL);
>>> - suppress_warning (r); /* We don't want a warning about this. */
>>> - r = maybe_cleanup_point_expr_void (r);
>>> - add_stmt (r);
>>> + /* Done. */
>>> + finish_return_stmt (NULL_TREE);
>>> /* This is the suspend return point. */
>>> - r = build_stmt (loc, LABEL_EXPR, ret_label);
>>> - add_stmt (r);
>>> + add_stmt (build_stmt (loc, LABEL_EXPR, ret_label));
>>> - r = build_stmt (loc, RETURN_EXPR, NULL);
>>> - suppress_warning (r); /* We don't want a warning about this. */
>>> - r = maybe_cleanup_point_expr_void (r);
>>> - add_stmt (r);
>>> + finish_return_stmt (NULL_TREE);
>>> /* This is the 'continuation' return point. For such a case we have a
>>> coro
>>> handle (from the await_suspend() call) and we want handle.resume() to
>>> execute as a tailcall allowing arbitrary chaining of coroutines. */
>>> - r = build_stmt (loc, LABEL_EXPR, continue_label);
>>> - add_stmt (r);
>>> + add_stmt (build_stmt (loc, LABEL_EXPR, continue_label));
>>> /* Should have been set earlier by the coro_initialized code. */
>>> gcc_assert (void_coro_handle_address);
