Hi Folks,
sorry for the long CC list - please feel free to ignore if you don’t care :)
I propose that this PR should be re-categorized as a “C++” one.
The reason is that this is not an oversight in the GCC implementation,
but a problem present in the general case. Library implementors feel
strongly that absence of symmetric transfer on important targets is a
Bad Thing.
======= possibilities to resolve this …..
We have, in the case of coroutine tail-calls, a useful factor in that all
such calls pass a single pointer, to the coroutine state frame. That
frame is initially set up in the DSO that spawns a given coroutine actor
function. Thus, at the point the frame is built, we have access to the
TOC, GOT, PLT for the relevant DSO.
It would seem excessive to resort to some kind of trampoline when we
already have somewhere to put the data we need to restore.
So .. what I’d like to do is to prototype a solution (probably on PPC) and
then take the necessary (coroutine) ABI amendments to the “coroutines
ABI group” (I am the editor of the current doc - which doesn’t have any
ps-component).
======= band-aid to fix the PR for stage 4.
tested on x86_64-linux, darwin (no loss of tail-call)
powerpc64-linux-gnu (tail call is OK on m32, and bypassed on m64)
solaris2.11 (tail call is bypassed on m32 and 64).
OK for master?
thanks
Iain
======= this is the commit message.
For symmetric transfers to work with C++20 coroutines, it is
currently necessary to tail call the callee coroutine from resume
method of the caller coroutine. The current codegen marks these
resume calls as "MUST_TAIL_CALL" to indicate that the tail call is
required for correctness.
Unfortunately, several targets have ABI constraints that prevent
an indirect tail-call, which results in the PRs compile error.
The change here tests the target sibcall hook for the resume
expression and only marks it as requiring a tail call if that's
supported.
This doesn't fix the underlying problem; that really a solution is
needed to allow the tail-calls (or equivalent) to take place - but
that will be deferred until next stage 1.
gcc/cp/ChangeLog:
2020-04-09 Iain Sandoe <i...@sandoe.co.uk>
PR c++/94359
* coroutines.cc (build_actor_fn): Check that the target can
support the resume tailcall before mandating it.
gcc/testsuite/ChangeLog:
2020-04-09 Iain Sandoe <i...@sandoe.co.uk>
* g++.dg/coroutines/torture/symmetric-transfer-00-basic.C:
Expect a run fail for targets without indirect tail-calls.
---
gcc/cp/coroutines.cc | 22 +++++++++++++------
.../torture/symmetric-transfer-00-basic.C | 3 ++-
2 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 57172853639..e4ba642d527 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2376,14 +2376,22 @@ build_actor_fn (location_t loc, tree coro_frame_type,
tree actor, tree fnbody,
tree resume = build_call_expr_loc
(loc, builtin_decl_explicit (BUILT_IN_CORO_RESUME), 1, addr);
+ /* In order to support an arbitrary number of coroutine continuations,
+ we must tail call them. However, some targets might not support this
+ for indirect calls, or calls between DSOs.
+ FIXME: see if there's an alternate strategy for such targets. */
/* Now we have the actual call, and we can mark it as a tail. */
CALL_EXPR_TAILCALL (resume) = true;
- /* ... and for optimisation levels 0..1, mark it as requiring a tail-call
- for correctness. It seems that doing this for optimisation levels that
- normally perform tail-calling, confuses the ME (or it would be logical
- to put this on unilaterally). */
- if (optimize < 2)
- CALL_EXPR_MUST_TAIL_CALL (resume) = true;
+ /* Temporarily, switch cfun so that we can use the target hook. */
+ push_struct_function (actor);
+ if (targetm.function_ok_for_sibcall (NULL_TREE, resume))
+ {
+ /* ... and for optimisation levels 0..1, which do not normally tail-
+ -call, mark it as requiring a tail-call for correctness. */
+ if (optimize < 2)
+ CALL_EXPR_MUST_TAIL_CALL (resume) = true;
+ }
+ pop_cfun ();
resume = coro_build_cvt_void_expr_stmt (resume, loc);
add_stmt (resume);
@@ -3951,7 +3959,7 @@ morph_fn_to_coro (tree orig, tree *resumer, tree
*destroyer)
push_deferring_access_checks (dk_no_check);
- /* Actor ... */
+ /* Build the actor... */
build_actor_fn (fn_start, coro_frame_type, actor, fnbody, orig, param_uses,
&local_var_uses, param_dtor_list, initial_await, final_await,
body_aw_points.await_number, frame_size);
diff --git
a/gcc/testsuite/g++.dg/coroutines/torture/symmetric-transfer-00-basic.C
b/gcc/testsuite/g++.dg/coroutines/torture/symmetric-transfer-00-basic.C
index 864846e365c..8211e8250ff 100644
--- a/gcc/testsuite/g++.dg/coroutines/torture/symmetric-transfer-00-basic.C
+++ b/gcc/testsuite/g++.dg/coroutines/torture/symmetric-transfer-00-basic.C
@@ -1,4 +1,5 @@
-// { dg-do run }
+// { dg-do run }
+// { dg-xfail-run-if "no indirect tailcall" { { lp64 && { powerpc64*-linux-gnu
} } || { *-*-solaris2* *-*-aix* } } }
#if __has_include(<coroutine>)
--
2.17.1
