| Message ID | ED4B8FAB-35C3-439A-B491-A303993063E4@sandoe.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | coroutines: Fix compile error with symmetric transfers [PR94359] | expand |
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
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(<coroutine>) 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
Hi Rainer, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> 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(<coroutine>) > > 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
Hi Iain, > Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> 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(<coroutine>) >> >> 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 <cpu wildcard>-*-*) for the rest. Rainer
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
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 <ro@CeBiTec.Uni-Bielefeld.DE> 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
Hi Rainer, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> 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
Hi Iain, > Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> 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 <ro@CeBiTec.Uni-Bielefeld.DE> wrote: >> Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> 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 <iain@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-14 Iain Sandoe <iain@sandoe.co.uk> 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/torture/symmetric-transfer-00-basic.C @@ -1,4 +1,10 @@ -// { dg-do run } +// { dg-do run } +// 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*-*-* } } #if __has_include(<coroutine>)
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>)