From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp001-out.apm-internet.net (smtp001-out.apm-internet.net [85.119.248.222]) by sourceware.org (Postfix) with ESMTPS id 2F76B3858C39 for ; Wed, 15 Sep 2021 18:32:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2F76B3858C39 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=sandoe.co.uk Authentication-Results: sourceware.org; spf=none smtp.mailfrom=sandoe.co.uk Received: (qmail 61818 invoked from network); 15 Sep 2021 18:32:38 -0000 X-APM-Out-ID: 16317307586181 X-APM-Authkey: 257869/1(257869/1) 4 Received: from unknown (HELO ?192.168.1.214?) (81.138.1.83) by smtp001.apm-internet.net with SMTP; 15 Sep 2021 18:32:38 -0000 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.21\)) Subject: Re: [PATCH] coroutines: Small cleanups to await_statement_walker [NFC]. From: Iain Sandoe In-Reply-To: <58a39ec9-cb56-c089-eaf2-3d43f317b272@redhat.com> Date: Wed, 15 Sep 2021 19:32:38 +0100 Cc: GCC Patches Content-Transfer-Encoding: quoted-printable Message-Id: <4233989E-BE46-48EC-A2D2-D541A5865B81@sandoe.co.uk> References: <732D8CCD-ABAE-43EB-B73C-D32E4553D84F@sandoe.co.uk> <58a39ec9-cb56-c089-eaf2-3d43f317b272@redhat.com> To: Jason Merrill X-Mailer: Apple Mail (2.3445.104.21) X-Spam-Status: No, score=-15.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_COUK, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Sep 2021 18:32:42 -0000 Hi Jason, > On 15 Sep 2021, at 18:32, Jason Merrill wrote: >=20 > On 9/14/21 11:36, Iain Sandoe wrote: >> Hi >> Some small code cleanups that allow us to have just one place that >> we handle a statement with await expression(s) embedded. Also we >> can reduce the work done to figure out whether a statement contains >> any such expressions. >> tested on x86_64,powerpc64le-linux x86_64-darwin >> OK for master? >> thanks >> Iain >> ----- >> There is no need to make a MODIFY_EXPR for any of the condition >> vars that we synthesize. >> Expansion of co_return can be carried out independently of any >> co_awaits that might be contained which simplifies this. >> Where we are rewriting statements to handle await expression >> logic, there is no need to carry out any analysis - we just need >> to detect the presence of any co_await. >> Signed-off-by: Iain Sandoe >> gcc/cp/ChangeLog: >> * coroutines.cc (await_statement_walker): Code cleanups. >> --- >> gcc/cp/coroutines.cc | 121 = ++++++++++++++++++++----------------------- >> 1 file changed, 56 insertions(+), 65 deletions(-) >> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >> index d2cc2e73c89..27556723b71 100644 >> --- a/gcc/cp/coroutines.cc >> +++ b/gcc/cp/coroutines.cc >> @@ -3412,16 +3412,11 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) >> return NULL_TREE; >> } >> - /* We have something to be handled as a single statement. */ >> - bool has_cleanup_wrapper =3D TREE_CODE (*stmt) =3D=3D = CLEANUP_POINT_EXPR; >> - hash_set visited; >> - awpts->saw_awaits =3D 0; >> - hash_set truth_aoif_to_expand; >> - awpts->truth_aoif_to_expand =3D &truth_aoif_to_expand; >> - awpts->needs_truth_if_exp =3D false; >> - awpts->has_awaiter_init =3D false; >> + /* We have something to be handled as a single statement. We have = to handle >> + a few statements specially where await statements have to be = moved out of >> + constructs. */ >> tree expr =3D *stmt; >> - if (has_cleanup_wrapper) >> + if (TREE_CODE (*stmt) =3D=3D CLEANUP_POINT_EXPR) >> expr =3D TREE_OPERAND (expr, 0); >> STRIP_NOPS (expr); >> @@ -3437,6 +3432,8 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) >> transforms can be implemented. */ >> case IF_STMT: >> { >> + tree *await_ptr; >> + hash_set visited; >> /* Transform 'if (cond with awaits) then stmt1 else stmt2' = into >> bool cond =3D cond with awaits. >> if (cond) then stmt1 else stmt2. */ >> @@ -3444,10 +3441,8 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) >> /* We treat the condition as if it was a stand-alone = statement, >> to see if there are any await expressions which will be = analyzed >> and registered. */ >> - if ((res =3D cp_walk_tree (&IF_COND (if_stmt), >> - analyze_expression_awaits, d, &visited))) >> - return res; >> - if (!awpts->saw_awaits) >> + if (!(cp_walk_tree (&IF_COND (if_stmt), >> + find_any_await, &await_ptr, &visited))) >> return NULL_TREE; /* Nothing special to do here. */ >> gcc_checking_assert (!awpts->bind_stack->is_empty()); >> @@ -3463,7 +3458,7 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) >> /* We want to initialize the new variable with the = expression >> that contains the await(s) and potentially also needs to >> have truth_if expressions expanded. */ >> - tree new_s =3D build2_loc (sloc, MODIFY_EXPR, = boolean_type_node, >> + tree new_s =3D build2_loc (sloc, INIT_EXPR, = boolean_type_node, >> newvar, cond_inner); >> finish_expr_stmt (new_s); >> IF_COND (if_stmt) =3D newvar; >> @@ -3477,25 +3472,25 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) >> break; >> case FOR_STMT: >> { >> + tree *await_ptr; >> + hash_set visited; >> /* for loops only need special treatment if the condition or = the >> iteration expression contain a co_await. */ >> tree for_stmt =3D *stmt; >> /* Sanity check. */ >> - if ((res =3D cp_walk_tree (&FOR_INIT_STMT (for_stmt), >> - analyze_expression_awaits, d, &visited))) >> - return res; >> - gcc_checking_assert (!awpts->saw_awaits); >> - >> - if ((res =3D cp_walk_tree (&FOR_COND (for_stmt), >> - analyze_expression_awaits, d, &visited))) >> - return res; >> - bool for_cond_await =3D awpts->saw_awaits !=3D 0; >> - unsigned save_awaits =3D awpts->saw_awaits; >> - >> - if ((res =3D cp_walk_tree (&FOR_EXPR (for_stmt), >> - analyze_expression_awaits, d, &visited))) >> - return res; >> - bool for_expr_await =3D awpts->saw_awaits > save_awaits; >> + gcc_checking_assert >> + (!(cp_walk_tree (&FOR_INIT_STMT (for_stmt), = find_any_await, >> + &await_ptr, &visited))); >=20 > What's the rationale for this assert? [expr.await] seems to say = explicitly that an await can appear in the initializer of an = init-statement. Indeed (and we would not expect otherwise) - but currently GCC appears to generate code for: for (loop_ind_var =3D init; =E2=80=A6 ; =E2=80=A6) {} that looks like: loop_ind_var =3D init; for (; =E2=80=A6 ; =E2=80=A6) {} If that changes (and the init contains an await expr) then we=E2=80=99d = need to apply that transform manually, so the assert is in place to = check that the assumption about existing behaviour is met. Iain >> + visited.empty (); >> + bool for_cond_await >> + =3D cp_walk_tree (&FOR_COND (for_stmt), find_any_await, >> + &await_ptr, &visited); >> + >> + visited.empty (); >> + bool for_expr_await >> + =3D cp_walk_tree (&FOR_EXPR (for_stmt), find_any_await, >> + &await_ptr, &visited); >> /* If the condition has an await, then we will need to = rewrite the >> loop as >> @@ -3538,7 +3533,12 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) >> =3D create_named_label_with_ctx (sloc, buf, = NULL_TREE); >> free (buf); >> add_stmt (build_stmt (sloc, LABEL_EXPR, it_expr_label)); >> - add_stmt (FOR_EXPR (for_stmt)); >> + tree for_expr =3D FOR_EXPR (for_stmt); >> + /* Present the iteration expression as a statement. */ >> + if (TREE_CODE (for_expr) =3D=3D CLEANUP_POINT_EXPR) >> + for_expr =3D TREE_OPERAND (for_expr, 0); >> + STRIP_NOPS (for_expr); >> + finish_expr_stmt (for_expr); >> FOR_EXPR (for_stmt) =3D NULL_TREE; >> FOR_BODY (for_stmt) =3D pop_stmt_list (insert_list); >> /* rewrite continue statements to goto label. */ >> @@ -3565,11 +3565,11 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) >> break; >> stmt.. >> } */ >> + tree *await_ptr; >> + hash_set visited; >> tree while_stmt =3D *stmt; >> - if ((res =3D cp_walk_tree (&WHILE_COND (while_stmt), >> - analyze_expression_awaits, d, &visited))) >> - return res; >> - if (!awpts->saw_awaits) >> + if (!(cp_walk_tree (&WHILE_COND (while_stmt), >> + find_any_await, &await_ptr, &visited))) >> return NULL_TREE; /* Nothing special to do here. */ >> tree insert_list =3D push_stmt_list (); >> @@ -3595,10 +3595,10 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) >> break; >> } while (true); */ >> tree do_stmt =3D *stmt; >> - if ((res =3D cp_walk_tree (&DO_COND (do_stmt), >> - analyze_expression_awaits, d, &visited))) >> - return res; >> - if (!awpts->saw_awaits) >> + tree *await_ptr; >> + hash_set visited; >> + if (!(cp_walk_tree (&DO_COND (do_stmt), >> + find_any_await, &await_ptr, &visited))) >> return NULL_TREE; /* Nothing special to do here. */ >> tree insert_list =3D push_stmt_list (); >> @@ -3621,10 +3621,10 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) >> switch_type cond =3D cond with awaits >> switch (cond) stmt. */ >> tree sw_stmt =3D *stmt; >> - if ((res =3D cp_walk_tree (&SWITCH_STMT_COND (sw_stmt), >> - analyze_expression_awaits, d, &visited))) >> - return res; >> - if (!awpts->saw_awaits) >> + tree *await_ptr; >> + hash_set visited; >> + if (!(cp_walk_tree (&SWITCH_STMT_COND (sw_stmt), >> + find_any_await, &await_ptr, &visited))) >> return NULL_TREE; /* Nothing special to do here. */ >> gcc_checking_assert (!awpts->bind_stack->is_empty()); >> @@ -3665,9 +3665,6 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) >> { expr; p.return_void(); goto final_suspend;} >> - for co_return [non void expr]; >> { p.return_value(expr); goto final_suspend; } */ >> - if ((res =3D cp_walk_tree (stmt, analyze_expression_awaits, >> - d, &visited))) >> - return res; >> location_t loc =3D EXPR_LOCATION (expr); >> tree call =3D TREE_OPERAND (expr, 1); >> expr =3D TREE_OPERAND (expr, 0); >> @@ -3675,39 +3672,33 @@ await_statement_walker (tree *stmt, int = *do_subtree, void *d) >> /* [stmt.return.coroutine], 2.2 >> If expr is present and void, it is placed immediately = before >> the call for return_void; */ >> - tree *maybe_await_stmt =3D NULL; >> if (expr && VOID_TYPE_P (TREE_TYPE (expr))) >> - { >> - finish_expr_stmt (expr); >> - /* If the return argument was a void expression, then = any >> - awaits must be contained in that. */ >> - maybe_await_stmt =3D tsi_stmt_ptr (tsi_last (ret_list)); >> - } >> + finish_expr_stmt (expr); >> /* Insert p.return_{void,value(expr)}. */ >> finish_expr_stmt (call); >> - /* Absent a return of a void expression, any awaits must be = in >> - the parameter to return_value(). */ >> - if (!maybe_await_stmt) >> - maybe_await_stmt =3D tsi_stmt_ptr (tsi_last (ret_list)); >> TREE_USED (awpts->fs_label) =3D 1; >> add_stmt (build_stmt (loc, GOTO_EXPR, awpts->fs_label)); >> *stmt =3D pop_stmt_list (ret_list); >> + res =3D cp_walk_tree (stmt, await_statement_walker, d, = NULL); >> /* Once this is complete, we will have processed subtrees. = */ >> *do_subtree =3D 0; >> - if (awpts->saw_awaits) >> - { >> - gcc_checking_assert (maybe_await_stmt); >> - res =3D cp_walk_tree (maybe_await_stmt, = await_statement_walker, >> - d, NULL); >> - if (res) >> - return res; >> - } >> - return NULL_TREE; /* Done. */ >> + return res; >> } >> break; >> } >> else if (EXPR_P (expr)) >> { >> + hash_set visited; >> + tree *await_ptr; >> + if (!(cp_walk_tree (stmt, find_any_await, &await_ptr, = &visited))) >> + return NULL_TREE; /* Nothing special to do here. */ >> + >> + visited.empty (); >> + awpts->saw_awaits =3D 0; >> + hash_set truth_aoif_to_expand; >> + awpts->truth_aoif_to_expand =3D &truth_aoif_to_expand; >> + awpts->needs_truth_if_exp =3D false; >> + awpts->has_awaiter_init =3D false; >> if ((res =3D cp_walk_tree (stmt, analyze_expression_awaits, d, = &visited))) >> return res; >> *do_subtree =3D 0; /* Done subtrees. */