coroutines : Avoid generating empty statements [96749].

Message ID 3E51A1E6-55E5-4AB3-A259-2E68184CF2C9@sandoe.co.uk
State New
Headers show
Series
  • coroutines : Avoid generating empty statements [96749].
Related show

Commit Message

Iain Sandoe March 15, 2021, 12:17 a.m.
Hi

In the compiler-only idiom:
" a = (target expr creates temp, op uses temp) "
the target expression variable needs to be promoted to a frame one
(if the expression has a suspend point).  However, the only uses of
the var are in the second part of the compound expression - and we
were creating an empty statement corresponding to the (now unused)
first arm.  This then produces the spurious warnings noted.

Fixed by avoiding generation of a separate variable nest for
isolated target expressions (or similarly isolated co_awaits used
in a function call).

tested on x86_64-darwin, x86_64-linux-gnu
and with cppcoro and folly/coroutines

OK for master/10.x?
thanks
Iain

gcc/cp/ChangeLog:

        * coroutines.cc (flatten_await_stmt): Allow for the case
        where a target expression variable only has uses in the
        second part of a compound expression.
        (maybe_promote_temps): Avoid emiting empty statements.

gcc/testsuite/ChangeLog:

	* g++.dg/coroutines/pr96749-1.C: New test.
	* g++.dg/coroutines/pr96749-2.C: New test.
---
 gcc/cp/coroutines.cc                        | 64 ++++++++++++++-------
 gcc/testsuite/g++.dg/coroutines/pr96749-1.C | 42 ++++++++++++++
 gcc/testsuite/g++.dg/coroutines/pr96749-2.C | 37 ++++++++++++
 3 files changed, 123 insertions(+), 20 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr96749-1.C
 create mode 100644 gcc/testsuite/g++.dg/coroutines/pr96749-2.C

-- 
2.24.1

Comments

Nathan Sidwell March 15, 2021, 11:48 a.m. | #1
On 3/14/21 8:17 PM, Iain Sandoe wrote:
> Hi

> 

> In the compiler-only idiom:

> " a = (target expr creates temp, op uses temp)"

> the target expression variable needs to be promoted to a frame one

> (if the expression has a suspend point).  However, the only uses of

> the var are in the second part of the compound expression - and we

> were creating an empty statement corresponding to the (now unused)

> first arm.  This then produces the spurious warnings noted.

> 

> Fixed by avoiding generation of a separate variable nest for

> isolated target expressions (or similarly isolated co_awaits used

> in a function call).

> 

> tested on x86_64-darwin, x86_64-linux-gnu

> and with cppcoro and folly/coroutines

> 

> OK for master/10.x?

> thanks

> Iain

> 

> gcc/cp/ChangeLog:

> 

>          * coroutines.cc (flatten_await_stmt): Allow for the case

>          where a target expression variable only has uses in the

>          second part of a compound expression.

>          (maybe_promote_temps): Avoid emiting empty statements.

> 



ok
> 



-- 
Nathan Sidwell

Patch

diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index c5aeb660ae8..712431583d5 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2955,7 +2955,9 @@  flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
 	break;
       case TARGET_EXPR:
 	{
-	  /* We have a temporary; promote it.  */
+	  /* We have a temporary; promote it, but allow for the idiom in code
+	     generated by the compiler like
+	     a = (target_expr produces temp, op uses temp).  */
 	  tree init = t;
 	  temps_used->add (init);
 	  tree var_type = TREE_TYPE (init);
@@ -2976,20 +2978,35 @@  flatten_await_stmt (var_nest_node *n, hash_set<tree> *promoted,
 	    }
 	  else
 	    init = build2 (INIT_EXPR, var_type, var, init);
-	  var_nest_node *ins
-	    = new var_nest_node (var, init, n->prev, n);
-	  /* We have to replace the target expr... */
-	  *v.entry = var;
-	  /* ... and any uses of its var.  */
-	  proxy_replace pr = {TREE_OPERAND (t, 0), var};
-	  cp_walk_tree (&n->init, replace_proxy, &pr, NULL);
-	  /* Compiler-generated temporaries can also have uses in following
-	     arms of compound expressions, which will be listed in 'replace_in'
-	     if present.  */
-	  if (replace_in)
-	    cp_walk_tree (replace_in, replace_proxy, &pr, NULL);
-	  flatten_await_stmt (ins, promoted, temps_used, NULL);
-	  flatten_await_stmt (n, promoted, temps_used, NULL);
+	  /* Simplify for the case that we have an init containing the temp
+	     alone.  */
+	  if (t == n->init && n->var == NULL_TREE)
+	    {
+	      n->var = var;
+	      proxy_replace pr = {TREE_OPERAND (t, 0), var};
+	      cp_walk_tree (&init, replace_proxy, &pr, NULL);
+	      n->init = init;
+	      if (replace_in)
+		cp_walk_tree (replace_in, replace_proxy, &pr, NULL);
+	      flatten_await_stmt (n, promoted, temps_used, NULL);
+	    }
+	  else
+	    {
+	      var_nest_node *ins
+		= new var_nest_node (var, init, n->prev, n);
+	      /* We have to replace the target expr... */
+	      *v.entry = var;
+	      /* ... and any uses of its var.  */
+	      proxy_replace pr = {TREE_OPERAND (t, 0), var};
+	      cp_walk_tree (&n->init, replace_proxy, &pr, NULL);
+	      /* Compiler-generated temporaries can also have uses in
+		 following arms of compound expressions, which will be listed
+		 in 'replace_in' if present.  */
+	      if (replace_in)
+		cp_walk_tree (replace_in, replace_proxy, &pr, NULL);
+	      flatten_await_stmt (ins, promoted, temps_used, NULL);
+	      flatten_await_stmt (n, promoted, temps_used, NULL);
+	    }
 	  return;
 	}
 	break;
@@ -3178,7 +3195,6 @@  maybe_promote_temps (tree *stmt, void *d)
   gcc_checking_assert (root->next == NULL);
   tree vlist = NULL_TREE;
   var_nest_node *t = root;
-  gcc_checking_assert (!t->var);
   /* We build the bind scope expression from the bottom-up.
      EXPR_LIST holds the inner expression nest at the current cleanup
      level (becoming the final expression list when we've exhausted the
@@ -3214,9 +3230,12 @@  maybe_promote_temps (tree *stmt, void *d)
 	      add_stmt (cl); /* push this onto the level above.  */
 	    }
 	  else if (expr_list)
-	    add_stmt (expr_list);
-	  else
-	    gcc_unreachable ();
+	    {
+	      if (TREE_CODE (expr_list) != STATEMENT_LIST)
+		add_stmt (expr_list);
+	      else if (!tsi_end_p (tsi_start (expr_list)))
+		add_stmt (expr_list);
+	    }
 	}
       else
 	{
@@ -3225,7 +3244,12 @@  maybe_promote_temps (tree *stmt, void *d)
 	  else
 	    finish_expr_stmt (t->init);
 	  if (expr_list)
-	    add_stmt (expr_list);
+	    {
+	      if (TREE_CODE (expr_list) != STATEMENT_LIST)
+		add_stmt (expr_list);
+	      else if (!tsi_end_p (tsi_start (expr_list)))
+		add_stmt (expr_list);
+	    }
 	}
       expr_list = pop_stmt_list (new_list);
       var_nest_node *old = t;
diff --git a/gcc/testsuite/g++.dg/coroutines/pr96749-1.C b/gcc/testsuite/g++.dg/coroutines/pr96749-1.C
new file mode 100644
index 00000000000..941a64e6379
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr96749-1.C
@@ -0,0 +1,42 @@ 
+//  { dg-additional-options "-Wall" }
+
+#include <coroutine>
+
+template <typename _Tp> struct promise;
+template <typename _Tp> struct task {
+	using promise_type = promise<_Tp>;
+	bool await_ready() noexcept { return false; }
+	void await_suspend(std::coroutine_handle<> awaiter) noexcept { m_a = awaiter; }
+	auto await_resume() { return _Tp(); }
+	std::coroutine_handle<> m_a;
+};
+
+template <typename _Tp> struct promise {
+	auto get_return_object() noexcept { return task<_Tp>(); }
+	auto initial_suspend() noexcept { return std::suspend_always(); }
+	auto final_suspend() noexcept { return std::suspend_always(); }
+	void return_value(_Tp value) noexcept { m_v = value; }
+	void unhandled_exception() noexcept {}
+	_Tp m_v;
+};
+
+task<int> test_coro(void) {
+	int r = 0;
+#if 1
+	// this code causes the unexpected warning below
+	r += co_await task<int>();
+#else
+	// this code causes no warning
+	auto b = co_await task<int>();
+	r += b;
+#endif
+	co_return r;
+	// test1.cpp: In function ‘task<int> test_coro(int)’:
+	// test1.cpp:36:1: warning: statement has no effect [-Wunused-value]
+	//   36 | }
+	//      | ^
+}
+
+int main(void) {
+	return 0;
+}
diff --git a/gcc/testsuite/g++.dg/coroutines/pr96749-2.C b/gcc/testsuite/g++.dg/coroutines/pr96749-2.C
new file mode 100644
index 00000000000..43052b57dd9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/coroutines/pr96749-2.C
@@ -0,0 +1,37 @@ 
+//  { dg-additional-options "-Wall" }
+
+#include <coroutine>
+
+#if 1
+// with a struct, GCC emits "statement has no effect"
+struct S {};
+#else
+// no warning with built-in types
+using S = int;
+#endif
+
+S Func1(int);
+
+struct C {
+	auto operator co_await() {
+		struct Awaitable final {
+			bool await_ready() const { return true; }
+			std::coroutine_handle<> await_suspend(std::coroutine_handle<>) { return {}; }
+			int await_resume() { return 42; }
+		};
+		return Awaitable{};
+	}
+};
+
+struct Task {
+	struct promise_type {
+		auto initial_suspend() { return std::suspend_always{}; }
+		auto final_suspend() noexcept { return std::suspend_always{}; }
+		void get_return_object() {}
+		void unhandled_exception() {}
+	};
+};
+
+Task Func2() {
+	Func1(co_await C());
+}