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

[Jason Merrill <jason@redhat.com>]

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