* [PATCH] coroutines: Small cleanups to await_statement_walker [NFC].
@ 2021-09-07 19:38 Iain Sandoe
0 siblings, 0 replies; 6+ messages in thread
From: Iain Sandoe @ 2021-09-07 19:38 UTC (permalink / raw)
To: GCC Patches
Hi,
This is a small code cleanup patch, but is useful in follow-on work to
fix actual bugs - by making it only one place that we need to consider
the flattening of a statement containing await expressions.
tested on x86-64, powerpc-linux, x86_64-darwin,
OK for master?
thanks
Iain
————— commit message
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 <iain@sandoe.co.uk>
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 = TREE_CODE (*stmt) == CLEANUP_POINT_EXPR;
- hash_set<tree> visited;
- awpts->saw_awaits = 0;
- hash_set<tree> truth_aoif_to_expand;
- awpts->truth_aoif_to_expand = &truth_aoif_to_expand;
- awpts->needs_truth_if_exp = false;
- awpts->has_awaiter_init = 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 = *stmt;
- if (has_cleanup_wrapper)
+ if (TREE_CODE (*stmt) == CLEANUP_POINT_EXPR)
expr = 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<tree> visited;
/* Transform 'if (cond with awaits) then stmt1 else stmt2' into
bool cond = 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 = 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 = build2_loc (sloc, MODIFY_EXPR, boolean_type_node,
+ tree new_s = build2_loc (sloc, INIT_EXPR, boolean_type_node,
newvar, cond_inner);
finish_expr_stmt (new_s);
IF_COND (if_stmt) = newvar;
@@ -3477,25 +3472,25 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
break;
case FOR_STMT:
{
+ tree *await_ptr;
+ hash_set<tree> visited;
/* for loops only need special treatment if the condition or the
iteration expression contain a co_await. */
tree for_stmt = *stmt;
/* Sanity check. */
- if ((res = cp_walk_tree (&FOR_INIT_STMT (for_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- gcc_checking_assert (!awpts->saw_awaits);
-
- if ((res = cp_walk_tree (&FOR_COND (for_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- bool for_cond_await = awpts->saw_awaits != 0;
- unsigned save_awaits = awpts->saw_awaits;
-
- if ((res = cp_walk_tree (&FOR_EXPR (for_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- bool for_expr_await = awpts->saw_awaits > save_awaits;
+ gcc_checking_assert
+ (!(cp_walk_tree (&FOR_INIT_STMT (for_stmt), find_any_await,
+ &await_ptr, &visited)));
+
+ visited.empty ();
+ bool for_cond_await
+ = cp_walk_tree (&FOR_COND (for_stmt), find_any_await,
+ &await_ptr, &visited);
+
+ visited.empty ();
+ bool for_expr_await
+ = 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)
= 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 = FOR_EXPR (for_stmt);
+ /* Present the iteration expression as a statement. */
+ if (TREE_CODE (for_expr) == CLEANUP_POINT_EXPR)
+ for_expr = TREE_OPERAND (for_expr, 0);
+ STRIP_NOPS (for_expr);
+ finish_expr_stmt (for_expr);
FOR_EXPR (for_stmt) = NULL_TREE;
FOR_BODY (for_stmt) = 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<tree> visited;
tree while_stmt = *stmt;
- if ((res = 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 = push_stmt_list ();
@@ -3595,10 +3595,10 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
break;
} while (true); */
tree do_stmt = *stmt;
- if ((res = cp_walk_tree (&DO_COND (do_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- if (!awpts->saw_awaits)
+ tree *await_ptr;
+ hash_set<tree> 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 = push_stmt_list ();
@@ -3621,10 +3621,10 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
switch_type cond = cond with awaits
switch (cond) stmt. */
tree sw_stmt = *stmt;
- if ((res = cp_walk_tree (&SWITCH_STMT_COND (sw_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- if (!awpts->saw_awaits)
+ tree *await_ptr;
+ hash_set<tree> 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 = cp_walk_tree (stmt, analyze_expression_awaits,
- d, &visited)))
- return res;
location_t loc = EXPR_LOCATION (expr);
tree call = TREE_OPERAND (expr, 1);
expr = 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 = 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 = 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 = tsi_stmt_ptr (tsi_last (ret_list));
TREE_USED (awpts->fs_label) = 1;
add_stmt (build_stmt (loc, GOTO_EXPR, awpts->fs_label));
*stmt = pop_stmt_list (ret_list);
+ res = cp_walk_tree (stmt, await_statement_walker, d, NULL);
/* Once this is complete, we will have processed subtrees. */
*do_subtree = 0;
- if (awpts->saw_awaits)
- {
- gcc_checking_assert (maybe_await_stmt);
- res = 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<tree> 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 = 0;
+ hash_set<tree> truth_aoif_to_expand;
+ awpts->truth_aoif_to_expand = &truth_aoif_to_expand;
+ awpts->needs_truth_if_exp = false;
+ awpts->has_awaiter_init = false;
if ((res = cp_walk_tree (stmt, analyze_expression_awaits, d, &visited)))
return res;
*do_subtree = 0; /* Done subtrees. */
--
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coroutines: Small cleanups to await_statement_walker [NFC].
2021-09-15 19:50 ` Jason Merrill
@ 2021-09-16 11:58 ` Iain Sandoe
0 siblings, 0 replies; 6+ messages in thread
From: Iain Sandoe @ 2021-09-16 11:58 UTC (permalink / raw)
To: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]
> On 15 Sep 2021, at 20:50, Jason Merrill <jason@redhat.com> wrote:
> On 9/15/21 14:32, Iain Sandoe wrote:
>> Hi Jason,
>>> On 15 Sep 2021, at 18:32, Jason Merrill <jason@redhat.com> wrote:
>>>
>>> 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
>>> 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 = init; … ; …) {}
>> that looks like:
>> loop_ind_var = init;
>> for (; … ; …) {}
>> If that changes (and the init contains an await expr) then we’d need to apply that transform manually, so the assert is in place to check that the assumption about existing behaviour is met.
>
> Then the patch is OK with that rationale in a comment.
thanks.
this is what was pushed:
[-- Attachment #2: 0001-coroutines-Small-cleanups-to-await_statement_walker-.patch --]
[-- Type: application/octet-stream, Size: 10087 bytes --]
From ab08859e37ef822c839bc33ec097091f17ebfe76 Mon Sep 17 00:00:00 2001
From: Iain Sandoe <iain@sandoe.co.uk>
Date: Wed, 14 Jul 2021 10:13:23 +0100
Subject: [PATCH] coroutines: Small cleanups to await_statement_walker [NFC].
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 <iain@sandoe.co.uk>
gcc/cp/ChangeLog:
* coroutines.cc (await_statement_walker): Code cleanups.
---
gcc/cp/coroutines.cc | 125 ++++++++++++++++++++-----------------------
1 file changed, 59 insertions(+), 66 deletions(-)
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index d2cc2e73c89..fbd5c49533f 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 = TREE_CODE (*stmt) == CLEANUP_POINT_EXPR;
- hash_set<tree> visited;
- awpts->saw_awaits = 0;
- hash_set<tree> truth_aoif_to_expand;
- awpts->truth_aoif_to_expand = &truth_aoif_to_expand;
- awpts->needs_truth_if_exp = false;
- awpts->has_awaiter_init = 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 = *stmt;
- if (has_cleanup_wrapper)
+ if (TREE_CODE (*stmt) == CLEANUP_POINT_EXPR)
expr = 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<tree> visited;
/* Transform 'if (cond with awaits) then stmt1 else stmt2' into
bool cond = 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 = 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 = build2_loc (sloc, MODIFY_EXPR, boolean_type_node,
+ tree new_s = build2_loc (sloc, INIT_EXPR, boolean_type_node,
newvar, cond_inner);
finish_expr_stmt (new_s);
IF_COND (if_stmt) = newvar;
@@ -3477,25 +3472,27 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
break;
case FOR_STMT:
{
+ tree *await_ptr;
+ hash_set<tree> visited;
/* for loops only need special treatment if the condition or the
iteration expression contain a co_await. */
tree for_stmt = *stmt;
- /* Sanity check. */
- if ((res = cp_walk_tree (&FOR_INIT_STMT (for_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- gcc_checking_assert (!awpts->saw_awaits);
-
- if ((res = cp_walk_tree (&FOR_COND (for_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- bool for_cond_await = awpts->saw_awaits != 0;
- unsigned save_awaits = awpts->saw_awaits;
-
- if ((res = cp_walk_tree (&FOR_EXPR (for_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- bool for_expr_await = awpts->saw_awaits > save_awaits;
+ /* At present, the FE always generates a separate initializer for
+ the FOR_INIT_STMT, when the expression has an await. Check that
+ this assumption holds in the future. */
+ gcc_checking_assert
+ (!(cp_walk_tree (&FOR_INIT_STMT (for_stmt), find_any_await,
+ &await_ptr, &visited)));
+
+ visited.empty ();
+ bool for_cond_await
+ = cp_walk_tree (&FOR_COND (for_stmt), find_any_await,
+ &await_ptr, &visited);
+
+ visited.empty ();
+ bool for_expr_await
+ = 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 +3535,12 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
= 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 = FOR_EXPR (for_stmt);
+ /* Present the iteration expression as a statement. */
+ if (TREE_CODE (for_expr) == CLEANUP_POINT_EXPR)
+ for_expr = TREE_OPERAND (for_expr, 0);
+ STRIP_NOPS (for_expr);
+ finish_expr_stmt (for_expr);
FOR_EXPR (for_stmt) = NULL_TREE;
FOR_BODY (for_stmt) = pop_stmt_list (insert_list);
/* rewrite continue statements to goto label. */
@@ -3565,11 +3567,11 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
break;
stmt..
} */
+ tree *await_ptr;
+ hash_set<tree> visited;
tree while_stmt = *stmt;
- if ((res = 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 = push_stmt_list ();
@@ -3595,10 +3597,10 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
break;
} while (true); */
tree do_stmt = *stmt;
- if ((res = cp_walk_tree (&DO_COND (do_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- if (!awpts->saw_awaits)
+ tree *await_ptr;
+ hash_set<tree> 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 = push_stmt_list ();
@@ -3621,10 +3623,10 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
switch_type cond = cond with awaits
switch (cond) stmt. */
tree sw_stmt = *stmt;
- if ((res = cp_walk_tree (&SWITCH_STMT_COND (sw_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- if (!awpts->saw_awaits)
+ tree *await_ptr;
+ hash_set<tree> 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 +3667,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 = cp_walk_tree (stmt, analyze_expression_awaits,
- d, &visited)))
- return res;
location_t loc = EXPR_LOCATION (expr);
tree call = TREE_OPERAND (expr, 1);
expr = TREE_OPERAND (expr, 0);
@@ -3675,39 +3674,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 = 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 = 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 = tsi_stmt_ptr (tsi_last (ret_list));
TREE_USED (awpts->fs_label) = 1;
add_stmt (build_stmt (loc, GOTO_EXPR, awpts->fs_label));
*stmt = pop_stmt_list (ret_list);
+ res = cp_walk_tree (stmt, await_statement_walker, d, NULL);
/* Once this is complete, we will have processed subtrees. */
*do_subtree = 0;
- if (awpts->saw_awaits)
- {
- gcc_checking_assert (maybe_await_stmt);
- res = 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<tree> 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 = 0;
+ hash_set<tree> truth_aoif_to_expand;
+ awpts->truth_aoif_to_expand = &truth_aoif_to_expand;
+ awpts->needs_truth_if_exp = false;
+ awpts->has_awaiter_init = false;
if ((res = cp_walk_tree (stmt, analyze_expression_awaits, d, &visited)))
return res;
*do_subtree = 0; /* Done subtrees. */
--
2.24.3 (Apple Git-128)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coroutines: Small cleanups to await_statement_walker [NFC].
2021-09-15 18:32 ` Iain Sandoe
@ 2021-09-15 19:50 ` Jason Merrill
2021-09-16 11:58 ` Iain Sandoe
0 siblings, 1 reply; 6+ messages in thread
From: Jason Merrill @ 2021-09-15 19:50 UTC (permalink / raw)
To: Iain Sandoe; +Cc: GCC Patches
On 9/15/21 14:32, Iain Sandoe wrote:
> Hi Jason,
>
>> On 15 Sep 2021, at 18:32, Jason Merrill <jason@redhat.com> wrote:
>>
>> 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 <iain@sandoe.co.uk>
>>> 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 = TREE_CODE (*stmt) == CLEANUP_POINT_EXPR;
>>> - hash_set<tree> visited;
>>> - awpts->saw_awaits = 0;
>>> - hash_set<tree> truth_aoif_to_expand;
>>> - awpts->truth_aoif_to_expand = &truth_aoif_to_expand;
>>> - awpts->needs_truth_if_exp = false;
>>> - awpts->has_awaiter_init = 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 = *stmt;
>>> - if (has_cleanup_wrapper)
>>> + if (TREE_CODE (*stmt) == CLEANUP_POINT_EXPR)
>>> expr = 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<tree> visited;
>>> /* Transform 'if (cond with awaits) then stmt1 else stmt2' into
>>> bool cond = 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 = 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 = build2_loc (sloc, MODIFY_EXPR, boolean_type_node,
>>> + tree new_s = build2_loc (sloc, INIT_EXPR, boolean_type_node,
>>> newvar, cond_inner);
>>> finish_expr_stmt (new_s);
>>> IF_COND (if_stmt) = newvar;
>>> @@ -3477,25 +3472,25 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
>>> break;
>>> case FOR_STMT:
>>> {
>>> + tree *await_ptr;
>>> + hash_set<tree> visited;
>>> /* for loops only need special treatment if the condition or the
>>> iteration expression contain a co_await. */
>>> tree for_stmt = *stmt;
>>> /* Sanity check. */
>>> - if ((res = cp_walk_tree (&FOR_INIT_STMT (for_stmt),
>>> - analyze_expression_awaits, d, &visited)))
>>> - return res;
>>> - gcc_checking_assert (!awpts->saw_awaits);
>>> -
>>> - if ((res = cp_walk_tree (&FOR_COND (for_stmt),
>>> - analyze_expression_awaits, d, &visited)))
>>> - return res;
>>> - bool for_cond_await = awpts->saw_awaits != 0;
>>> - unsigned save_awaits = awpts->saw_awaits;
>>> -
>>> - if ((res = cp_walk_tree (&FOR_EXPR (for_stmt),
>>> - analyze_expression_awaits, d, &visited)))
>>> - return res;
>>> - bool for_expr_await = awpts->saw_awaits > save_awaits;
>>> + gcc_checking_assert
>>> + (!(cp_walk_tree (&FOR_INIT_STMT (for_stmt), find_any_await,
>>> + &await_ptr, &visited)));
>>
>> 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 = init; … ; …) {}
>
> that looks like:
>
> loop_ind_var = init;
> for (; … ; …) {}
>
> If that changes (and the init contains an await expr) then we’d need to apply that transform manually, so the assert is in place to check that the assumption about existing behaviour is met.
Then the patch is OK with that rationale in a comment.
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coroutines: Small cleanups to await_statement_walker [NFC].
2021-09-15 17:32 ` Jason Merrill
@ 2021-09-15 18:32 ` Iain Sandoe
2021-09-15 19:50 ` Jason Merrill
0 siblings, 1 reply; 6+ messages in thread
From: Iain Sandoe @ 2021-09-15 18:32 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
Hi Jason,
> On 15 Sep 2021, at 18:32, Jason Merrill <jason@redhat.com> wrote:
>
> 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 <iain@sandoe.co.uk>
>> 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 = TREE_CODE (*stmt) == CLEANUP_POINT_EXPR;
>> - hash_set<tree> visited;
>> - awpts->saw_awaits = 0;
>> - hash_set<tree> truth_aoif_to_expand;
>> - awpts->truth_aoif_to_expand = &truth_aoif_to_expand;
>> - awpts->needs_truth_if_exp = false;
>> - awpts->has_awaiter_init = 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 = *stmt;
>> - if (has_cleanup_wrapper)
>> + if (TREE_CODE (*stmt) == CLEANUP_POINT_EXPR)
>> expr = 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<tree> visited;
>> /* Transform 'if (cond with awaits) then stmt1 else stmt2' into
>> bool cond = 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 = 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 = build2_loc (sloc, MODIFY_EXPR, boolean_type_node,
>> + tree new_s = build2_loc (sloc, INIT_EXPR, boolean_type_node,
>> newvar, cond_inner);
>> finish_expr_stmt (new_s);
>> IF_COND (if_stmt) = newvar;
>> @@ -3477,25 +3472,25 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
>> break;
>> case FOR_STMT:
>> {
>> + tree *await_ptr;
>> + hash_set<tree> visited;
>> /* for loops only need special treatment if the condition or the
>> iteration expression contain a co_await. */
>> tree for_stmt = *stmt;
>> /* Sanity check. */
>> - if ((res = cp_walk_tree (&FOR_INIT_STMT (for_stmt),
>> - analyze_expression_awaits, d, &visited)))
>> - return res;
>> - gcc_checking_assert (!awpts->saw_awaits);
>> -
>> - if ((res = cp_walk_tree (&FOR_COND (for_stmt),
>> - analyze_expression_awaits, d, &visited)))
>> - return res;
>> - bool for_cond_await = awpts->saw_awaits != 0;
>> - unsigned save_awaits = awpts->saw_awaits;
>> -
>> - if ((res = cp_walk_tree (&FOR_EXPR (for_stmt),
>> - analyze_expression_awaits, d, &visited)))
>> - return res;
>> - bool for_expr_await = awpts->saw_awaits > save_awaits;
>> + gcc_checking_assert
>> + (!(cp_walk_tree (&FOR_INIT_STMT (for_stmt), find_any_await,
>> + &await_ptr, &visited)));
>
> 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 = init; … ; …) {}
that looks like:
loop_ind_var = init;
for (; … ; …) {}
If that changes (and the init contains an await expr) then we’d 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
>> + = cp_walk_tree (&FOR_COND (for_stmt), find_any_await,
>> + &await_ptr, &visited);
>> +
>> + visited.empty ();
>> + bool for_expr_await
>> + = 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)
>> = 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 = FOR_EXPR (for_stmt);
>> + /* Present the iteration expression as a statement. */
>> + if (TREE_CODE (for_expr) == CLEANUP_POINT_EXPR)
>> + for_expr = TREE_OPERAND (for_expr, 0);
>> + STRIP_NOPS (for_expr);
>> + finish_expr_stmt (for_expr);
>> FOR_EXPR (for_stmt) = NULL_TREE;
>> FOR_BODY (for_stmt) = 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<tree> visited;
>> tree while_stmt = *stmt;
>> - if ((res = 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 = push_stmt_list ();
>> @@ -3595,10 +3595,10 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
>> break;
>> } while (true); */
>> tree do_stmt = *stmt;
>> - if ((res = cp_walk_tree (&DO_COND (do_stmt),
>> - analyze_expression_awaits, d, &visited)))
>> - return res;
>> - if (!awpts->saw_awaits)
>> + tree *await_ptr;
>> + hash_set<tree> 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 = push_stmt_list ();
>> @@ -3621,10 +3621,10 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
>> switch_type cond = cond with awaits
>> switch (cond) stmt. */
>> tree sw_stmt = *stmt;
>> - if ((res = cp_walk_tree (&SWITCH_STMT_COND (sw_stmt),
>> - analyze_expression_awaits, d, &visited)))
>> - return res;
>> - if (!awpts->saw_awaits)
>> + tree *await_ptr;
>> + hash_set<tree> 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 = cp_walk_tree (stmt, analyze_expression_awaits,
>> - d, &visited)))
>> - return res;
>> location_t loc = EXPR_LOCATION (expr);
>> tree call = TREE_OPERAND (expr, 1);
>> expr = 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 = 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 = 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 = tsi_stmt_ptr (tsi_last (ret_list));
>> TREE_USED (awpts->fs_label) = 1;
>> add_stmt (build_stmt (loc, GOTO_EXPR, awpts->fs_label));
>> *stmt = pop_stmt_list (ret_list);
>> + res = cp_walk_tree (stmt, await_statement_walker, d, NULL);
>> /* Once this is complete, we will have processed subtrees. */
>> *do_subtree = 0;
>> - if (awpts->saw_awaits)
>> - {
>> - gcc_checking_assert (maybe_await_stmt);
>> - res = 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<tree> 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 = 0;
>> + hash_set<tree> truth_aoif_to_expand;
>> + awpts->truth_aoif_to_expand = &truth_aoif_to_expand;
>> + awpts->needs_truth_if_exp = false;
>> + awpts->has_awaiter_init = false;
>> if ((res = cp_walk_tree (stmt, analyze_expression_awaits, d, &visited)))
>> return res;
>> *do_subtree = 0; /* Done subtrees. */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coroutines: Small cleanups to await_statement_walker [NFC].
2021-09-14 15:36 Iain Sandoe
@ 2021-09-15 17:32 ` Jason Merrill
2021-09-15 18:32 ` Iain Sandoe
0 siblings, 1 reply; 6+ messages in thread
From: Jason Merrill @ 2021-09-15 17:32 UTC (permalink / raw)
To: Iain Sandoe, GCC Patches
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 <iain@sandoe.co.uk>
>
> 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 = TREE_CODE (*stmt) == CLEANUP_POINT_EXPR;
> - hash_set<tree> visited;
> - awpts->saw_awaits = 0;
> - hash_set<tree> truth_aoif_to_expand;
> - awpts->truth_aoif_to_expand = &truth_aoif_to_expand;
> - awpts->needs_truth_if_exp = false;
> - awpts->has_awaiter_init = 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 = *stmt;
> - if (has_cleanup_wrapper)
> + if (TREE_CODE (*stmt) == CLEANUP_POINT_EXPR)
> expr = 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<tree> visited;
> /* Transform 'if (cond with awaits) then stmt1 else stmt2' into
> bool cond = 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 = 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 = build2_loc (sloc, MODIFY_EXPR, boolean_type_node,
> + tree new_s = build2_loc (sloc, INIT_EXPR, boolean_type_node,
> newvar, cond_inner);
> finish_expr_stmt (new_s);
> IF_COND (if_stmt) = newvar;
> @@ -3477,25 +3472,25 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
> break;
> case FOR_STMT:
> {
> + tree *await_ptr;
> + hash_set<tree> visited;
> /* for loops only need special treatment if the condition or the
> iteration expression contain a co_await. */
> tree for_stmt = *stmt;
> /* Sanity check. */
> - if ((res = cp_walk_tree (&FOR_INIT_STMT (for_stmt),
> - analyze_expression_awaits, d, &visited)))
> - return res;
> - gcc_checking_assert (!awpts->saw_awaits);
> -
> - if ((res = cp_walk_tree (&FOR_COND (for_stmt),
> - analyze_expression_awaits, d, &visited)))
> - return res;
> - bool for_cond_await = awpts->saw_awaits != 0;
> - unsigned save_awaits = awpts->saw_awaits;
> -
> - if ((res = cp_walk_tree (&FOR_EXPR (for_stmt),
> - analyze_expression_awaits, d, &visited)))
> - return res;
> - bool for_expr_await = awpts->saw_awaits > save_awaits;
> + gcc_checking_assert
> + (!(cp_walk_tree (&FOR_INIT_STMT (for_stmt), find_any_await,
> + &await_ptr, &visited)));
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.
> + visited.empty ();
> + bool for_cond_await
> + = cp_walk_tree (&FOR_COND (for_stmt), find_any_await,
> + &await_ptr, &visited);
> +
> + visited.empty ();
> + bool for_expr_await
> + = 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)
> = 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 = FOR_EXPR (for_stmt);
> + /* Present the iteration expression as a statement. */
> + if (TREE_CODE (for_expr) == CLEANUP_POINT_EXPR)
> + for_expr = TREE_OPERAND (for_expr, 0);
> + STRIP_NOPS (for_expr);
> + finish_expr_stmt (for_expr);
> FOR_EXPR (for_stmt) = NULL_TREE;
> FOR_BODY (for_stmt) = 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<tree> visited;
> tree while_stmt = *stmt;
> - if ((res = 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 = push_stmt_list ();
> @@ -3595,10 +3595,10 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
> break;
> } while (true); */
> tree do_stmt = *stmt;
> - if ((res = cp_walk_tree (&DO_COND (do_stmt),
> - analyze_expression_awaits, d, &visited)))
> - return res;
> - if (!awpts->saw_awaits)
> + tree *await_ptr;
> + hash_set<tree> 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 = push_stmt_list ();
> @@ -3621,10 +3621,10 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
> switch_type cond = cond with awaits
> switch (cond) stmt. */
> tree sw_stmt = *stmt;
> - if ((res = cp_walk_tree (&SWITCH_STMT_COND (sw_stmt),
> - analyze_expression_awaits, d, &visited)))
> - return res;
> - if (!awpts->saw_awaits)
> + tree *await_ptr;
> + hash_set<tree> 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 = cp_walk_tree (stmt, analyze_expression_awaits,
> - d, &visited)))
> - return res;
> location_t loc = EXPR_LOCATION (expr);
> tree call = TREE_OPERAND (expr, 1);
> expr = 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 = 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 = 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 = tsi_stmt_ptr (tsi_last (ret_list));
> TREE_USED (awpts->fs_label) = 1;
> add_stmt (build_stmt (loc, GOTO_EXPR, awpts->fs_label));
> *stmt = pop_stmt_list (ret_list);
> + res = cp_walk_tree (stmt, await_statement_walker, d, NULL);
> /* Once this is complete, we will have processed subtrees. */
> *do_subtree = 0;
> - if (awpts->saw_awaits)
> - {
> - gcc_checking_assert (maybe_await_stmt);
> - res = 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<tree> 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 = 0;
> + hash_set<tree> truth_aoif_to_expand;
> + awpts->truth_aoif_to_expand = &truth_aoif_to_expand;
> + awpts->needs_truth_if_exp = false;
> + awpts->has_awaiter_init = false;
> if ((res = cp_walk_tree (stmt, analyze_expression_awaits, d, &visited)))
> return res;
> *do_subtree = 0; /* Done subtrees. */
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] coroutines: Small cleanups to await_statement_walker [NFC].
@ 2021-09-14 15:36 Iain Sandoe
2021-09-15 17:32 ` Jason Merrill
0 siblings, 1 reply; 6+ messages in thread
From: Iain Sandoe @ 2021-09-14 15:36 UTC (permalink / raw)
To: GCC Patches
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 <iain@sandoe.co.uk>
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 = TREE_CODE (*stmt) == CLEANUP_POINT_EXPR;
- hash_set<tree> visited;
- awpts->saw_awaits = 0;
- hash_set<tree> truth_aoif_to_expand;
- awpts->truth_aoif_to_expand = &truth_aoif_to_expand;
- awpts->needs_truth_if_exp = false;
- awpts->has_awaiter_init = 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 = *stmt;
- if (has_cleanup_wrapper)
+ if (TREE_CODE (*stmt) == CLEANUP_POINT_EXPR)
expr = 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<tree> visited;
/* Transform 'if (cond with awaits) then stmt1 else stmt2' into
bool cond = 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 = 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 = build2_loc (sloc, MODIFY_EXPR, boolean_type_node,
+ tree new_s = build2_loc (sloc, INIT_EXPR, boolean_type_node,
newvar, cond_inner);
finish_expr_stmt (new_s);
IF_COND (if_stmt) = newvar;
@@ -3477,25 +3472,25 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
break;
case FOR_STMT:
{
+ tree *await_ptr;
+ hash_set<tree> visited;
/* for loops only need special treatment if the condition or the
iteration expression contain a co_await. */
tree for_stmt = *stmt;
/* Sanity check. */
- if ((res = cp_walk_tree (&FOR_INIT_STMT (for_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- gcc_checking_assert (!awpts->saw_awaits);
-
- if ((res = cp_walk_tree (&FOR_COND (for_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- bool for_cond_await = awpts->saw_awaits != 0;
- unsigned save_awaits = awpts->saw_awaits;
-
- if ((res = cp_walk_tree (&FOR_EXPR (for_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- bool for_expr_await = awpts->saw_awaits > save_awaits;
+ gcc_checking_assert
+ (!(cp_walk_tree (&FOR_INIT_STMT (for_stmt), find_any_await,
+ &await_ptr, &visited)));
+
+ visited.empty ();
+ bool for_cond_await
+ = cp_walk_tree (&FOR_COND (for_stmt), find_any_await,
+ &await_ptr, &visited);
+
+ visited.empty ();
+ bool for_expr_await
+ = 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)
= 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 = FOR_EXPR (for_stmt);
+ /* Present the iteration expression as a statement. */
+ if (TREE_CODE (for_expr) == CLEANUP_POINT_EXPR)
+ for_expr = TREE_OPERAND (for_expr, 0);
+ STRIP_NOPS (for_expr);
+ finish_expr_stmt (for_expr);
FOR_EXPR (for_stmt) = NULL_TREE;
FOR_BODY (for_stmt) = 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<tree> visited;
tree while_stmt = *stmt;
- if ((res = 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 = push_stmt_list ();
@@ -3595,10 +3595,10 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
break;
} while (true); */
tree do_stmt = *stmt;
- if ((res = cp_walk_tree (&DO_COND (do_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- if (!awpts->saw_awaits)
+ tree *await_ptr;
+ hash_set<tree> 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 = push_stmt_list ();
@@ -3621,10 +3621,10 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d)
switch_type cond = cond with awaits
switch (cond) stmt. */
tree sw_stmt = *stmt;
- if ((res = cp_walk_tree (&SWITCH_STMT_COND (sw_stmt),
- analyze_expression_awaits, d, &visited)))
- return res;
- if (!awpts->saw_awaits)
+ tree *await_ptr;
+ hash_set<tree> 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 = cp_walk_tree (stmt, analyze_expression_awaits,
- d, &visited)))
- return res;
location_t loc = EXPR_LOCATION (expr);
tree call = TREE_OPERAND (expr, 1);
expr = 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 = 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 = 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 = tsi_stmt_ptr (tsi_last (ret_list));
TREE_USED (awpts->fs_label) = 1;
add_stmt (build_stmt (loc, GOTO_EXPR, awpts->fs_label));
*stmt = pop_stmt_list (ret_list);
+ res = cp_walk_tree (stmt, await_statement_walker, d, NULL);
/* Once this is complete, we will have processed subtrees. */
*do_subtree = 0;
- if (awpts->saw_awaits)
- {
- gcc_checking_assert (maybe_await_stmt);
- res = 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<tree> 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 = 0;
+ hash_set<tree> truth_aoif_to_expand;
+ awpts->truth_aoif_to_expand = &truth_aoif_to_expand;
+ awpts->needs_truth_if_exp = false;
+ awpts->has_awaiter_init = false;
if ((res = cp_walk_tree (stmt, analyze_expression_awaits, d, &visited)))
return res;
*do_subtree = 0; /* Done subtrees. */
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-16 11:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-07 19:38 [PATCH] coroutines: Small cleanups to await_statement_walker [NFC] Iain Sandoe
2021-09-14 15:36 Iain Sandoe
2021-09-15 17:32 ` Jason Merrill
2021-09-15 18:32 ` Iain Sandoe
2021-09-15 19:50 ` Jason Merrill
2021-09-16 11:58 ` Iain Sandoe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).