Re: [PATCH] coroutines: Small cleanups to await_statement_walker [NFC].

[Iain Sandoe <iain@sandoe.co.uk>]

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.  */