Re: [PATCH] coroutines: Fix compile error with symmetric transfers [PR94359]
Rainer Orth wrote:
>> Rainer Orth wrote:
On Sat, Apr 11, 2020 at 03:46:18PM +0100, Iain Sandoe wrote:
> Unfortunately, several targets have ABI constraints that prevent
> an indirect tail-call, which results in the PRs compile error.
>>
>> So .. after scanning the current published testsuite results I have:
>>
>> // See PR94359 - some targets are unable to make general indirect tailcalls
>> // for example, between different DSOs.
>> // { dg-xfail-run-if "" { hppa*-*-hpux11* } }
>> // { dg-xfail-run-if "" { ia64-*-linux-gnu } }
>> // { dg-xfail-run-if "" { { lp64 && { powerpc*-linux-gnu } } || { *-*-aix*
>> } } }
>> // { dg-xfail-run-if "" { sparc-*-solaris2* } }
>
> this is wrong: for one, it needs to allow for 64-bit-default SPARC
> configurations (i.e. sparcv9-*-* and sparc64-*-*), and AFAICS there's
> nothing Solaris-specific here. So this should be sparc*-*-* instead.
>
>> I’d say it would be a reasonable idea to get this in sooner rather than
>> later, we
>> can always increase the list (or a target maintainer can trivially add
>> their target)
>
> Fine with me. If the list gets much longer or the conditions more
> complex, it might still be worthwhile to use an effective-target keyword
> instead.
This is what I’ve pushed after re-checking on
x86_64-linux/darwin
sparc-solaris11
powerpc64-linux-gnu
I won’t be entirely surprised if the list needs amendment or additions
but that’s realistically what I can test.
thanks for the reviews,
Iain
coroutines: Fix compile error with symmetric transfers [PR94359]
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-14 Iain Sandoe
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-14 Iain Sandoe
PR c++/94359
* g++.dg/coroutines/torture/symmetric-transfer-00-basic.C:
Expect a run fail for targets without arbitrary indirect
tail-calls.
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..6f379c8e77a 100644
--- a/gcc/testsuite/g++.dg/coroutines/torture/symmetric-transfer-00-basic.C
+++ b/gcc/testsuite/g++.dg/coroutines/tort
Re: [PATCH] coroutines: Fix compile error with symmetric transfers [PR94359]
Hi Iain,
> Rainer Orth wrote:
>>>
>>> On Sat, Apr 11, 2020 at 03:46:18PM +0100, Iain Sandoe wrote:
Unfortunately, several targets have ABI constraints that prevent
an indirect tail-call, which results in the PRs compile error.
>
> So .. after scanning the current published testsuite results I have:
>
> // See PR94359 - some targets are unable to make general indirect tailcalls
> // for example, between different DSOs.
> // { dg-xfail-run-if "" { hppa*-*-hpux11* } }
> // { dg-xfail-run-if "" { ia64-*-linux-gnu } }
> // { dg-xfail-run-if "" { { lp64 && { powerpc*-linux-gnu } } || { *-*-aix*
> } } }
> // { dg-xfail-run-if "" { sparc-*-solaris2* } }
this is wrong: for one, it needs to allow for 64-bit-default SPARC
configurations (i.e. sparcv9-*-* and sparc64-*-*), and AFAICS there's
nothing Solaris-specific here. So this should be sparc*-*-* instead.
> I’d say it would be a reasonable idea to get this in sooner rather than
> later, we
> can always increase the list (or a target maintainer can trivially add
> their target)
Fine with me. If the list gets much longer or the conditions more
complex, it might still be worthwhile to use an effective-target keyword
instead.
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] coroutines: Fix compile error with symmetric transfers [PR94359]
Hi Rainer,
Rainer Orth wrote:
On Sat, Apr 11, 2020 at 03:46:18PM +0100, Iain Sandoe wrote:
Unfortunately, several targets have ABI constraints that prevent
an indirect tail-call, which results in the PRs compile error.
So .. after scanning the current published testsuite results I have:
// See PR94359 - some targets are unable to make general indirect tailcalls
// for example, between different DSOs.
// { dg-xfail-run-if "" { hppa*-*-hpux11* } }
// { dg-xfail-run-if "" { ia64-*-linux-gnu } }
// { dg-xfail-run-if "" { { lp64 && { powerpc*-linux-gnu } } || { *-*-aix*
} } }
// { dg-xfail-run-if "" { sparc-*-solaris2* } }
* I don’t see the armv8l- fail in the results.
* hppa-linux seems OK.
* I’m avoiding writing powerpc and rs6000 aix, since I expect the result to
be
the same for both.
I’d say it would be a reasonable idea to get this in sooner rather than
later, we
can always increase the list (or a target maintainer can trivially add
their target)
happy to test on the platforms I have access to if this seems a reasonable
start
thanks
Iain
Re: [PATCH] coroutines: Fix compile error with symmetric transfers [PR94359]
Rainer Orth wrote:
Hi Segher,
On Sat, Apr 11, 2020 at 03:46:18PM +0100, Iain Sandoe wrote:
Unfortunately, several targets have ABI constraints that prevent
an indirect tail-call, which results in the PRs compile error.
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* } } }
lp64 && powerpc*-*-linux (we have biarch compilers :-) )
The problem is not that there is no indirect tailcall; the problem is
that no tailcall can be done to a routine that (potentially) has a
different TOC.
From rs6000_function_ok_for_sibcall:
Under the AIX or ELFv2 ABIs we can't allow calls to non-local
functions, because the callee may have a different TOC pointer to
the caller and there's no way to ensure we restore the TOC when
we return.
so shouldn't the above be
lp64 && powerpc*-*-*
instead to cover AIX, too? Or are there other PowerPC ABIs that don't
have this issue?
Just for once, Darwin (both PPC and X86) has no problem (doesn’t use the TOC
and has a per-function “got”), so that’s one.
Iain
Re: [PATCH] coroutines: Fix compile error with symmetric transfers [PR94359]
Hi Segher,
> On Sat, Apr 11, 2020 at 03:46:18PM +0100, Iain Sandoe wrote:
>> Unfortunately, several targets have ABI constraints that prevent
>> an indirect tail-call, which results in the PRs compile error.
>
>> 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* } } }
>
> lp64 && powerpc*-*-linux (we have biarch compilers :-) )
>
> The problem is not that there is no indirect tailcall; the problem is
> that no tailcall can be done to a routine that (potentially) has a
> different TOC.
>
> From rs6000_function_ok_for_sibcall:
> Under the AIX or ELFv2 ABIs we can't allow calls to non-local
> functions, because the callee may have a different TOC pointer to
> the caller and there's no way to ensure we restore the TOC when
> we return.
so shouldn't the above be
lp64 && powerpc*-*-*
instead to cover AIX, too? Or are there other PowerPC ABIs that don't
have this issue?
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] coroutines: Fix compile error with symmetric transfers [PR94359]
Hi!
On Sat, Apr 11, 2020 at 03:46:18PM +0100, Iain Sandoe wrote:
> Unfortunately, several targets have ABI constraints that prevent
> an indirect tail-call, which results in the PRs compile error.
> 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* } } }
lp64 && powerpc*-*-linux (we have biarch compilers :-) )
The problem is not that there is no indirect tailcall; the problem is
that no tailcall can be done to a routine that (potentially) has a
different TOC.
>From rs6000_function_ok_for_sibcall:
Under the AIX or ELFv2 ABIs we can't allow calls to non-local
functions, because the callee may have a different TOC pointer to
the caller and there's no way to ensure we restore the TOC when
we return.
Segher
Re: [PATCH] coroutines: Fix compile error with symmetric transfers [PR94359]
Hi Iain,
> Rainer Orth wrote:
>
>>> 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()
>>
>> unfortunately, the dg-xfail-run-if is wrong. E.g. it causes XPASSes on
>> i386-pc-solaris2.11.
>
> .. so that behaves in a similar way to PPC?
> fail on m64 pass on m32?
> (I don’t have access to any x86 solaris testing)
no, it PASSes for 32 and 64-bit alike, just as e.g. Linux/x86_64.
> according to my testing, sparc solaris fails for m32 and m64 (so the
> condition doesn’t need any multilib discriminator there)
Right, same here.
>> You should base this on the cpu part of the triplet in general, not on
>> the OS. Besides, according to gcc-testresults postings, the test FAILs
>> on other targets as well: armv8l-unknown-linux-gnueabihf, hppa*, and ia64.
>>
>> Besides, unless you want to introduce an effective-target keyword (with
>> documentation in sourcebuild.texi), probably overkill for a single use,
>> you can have more than one dg-xfail-run-if line to improve readibility.
>
> I’ll take a look at those and make multiple xfail-run-if’s
> (one per target might be the neatest, and easier for a target maintainer to
> add if one is missed).
I thought about separate ones for cases that need additional conditions
and an alphabetical list of target triplets (just -*-*)
for the rest.
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] coroutines: Fix compile error with symmetric transfers [PR94359]
Hi Rainer,
Rainer Orth wrote:
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()
unfortunately, the dg-xfail-run-if is wrong. E.g. it causes XPASSes on
i386-pc-solaris2.11.
.. so that behaves in a similar way to PPC?
fail on m64 pass on m32?
(I don’t have access to any x86 solaris testing)
according to my testing, sparc solaris fails for m32 and m64 (so the
condition doesn’t need any multilib discriminator there)
You should base this on the cpu part of the triplet in general, not on
the OS. Besides, according to gcc-testresults postings, the test FAILs
on other targets as well: armv8l-unknown-linux-gnueabihf, hppa*, and ia64.
Besides, unless you want to introduce an effective-target keyword (with
documentation in sourcebuild.texi), probably overkill for a single use,
you can have more than one dg-xfail-run-if line to improve readibility.
I’ll take a look at those and make multiple xfail-run-if’s
(one per target might be the neatest, and easier for a target maintainer to
add if one is missed).
thanks
Iain
Re: [PATCH] coroutines: Fix compile error with symmetric transfers [PR94359]
Hi Iain,
> 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()
unfortunately, the dg-xfail-run-if is wrong. E.g. it causes XPASSes on
i386-pc-solaris2.11.
You should base this on the cpu part of the triplet in general, not on
the OS. Besides, according to gcc-testresults postings, the test FAILs
on other targets as well: armv8l-unknown-linux-gnueabihf, hppa*, and ia64.
Besides, unless you want to introduce an effective-target keyword (with
documentation in sourcebuild.texi), probably overkill for a single use,
you can have more than one dg-xfail-run-if line to improve readibility.
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] coroutines: Fix compile error with symmetric transfers [PR94359]
On 4/11/20 10:46 AM, Iain Sandoe wrote: 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. This is fine from my PoV for gcc 10. nathan -- Nathan Sidwell
