Re: [PATCH] coroutines: Correct frame capture of compiler temps [PR95591+4].
Iain Sandoe wrote:
Iain Sandoe wrote:
Jason Merrill wrote:
I notice this patch includes
+ var_nest_node () = default;
which will break GCC 10 bootstrap with a C++98 compiler; we only
switched to C++11 for GCC 11.
Hmm, the patch was already backported…
… I will fix this.
I missed the warning during testing.
We set -std=gnu++98 for the stage1 compiler, which does warn about this
(although there
is a lot of warning output from a bootstrap, it’s easy to miss one).
I’m mostly bootstrapping with GCC-7.5 and it appears that this does not
actually produce
any error even if one actually uses the defaulted ctor (at least in a
trivial test).
= anyway ….
OK for gcc-10?
Nathan acked this on IRC, so I pushed now.
thanks
Iain
this removes the warning from the stage #1.
thanks
Iain
==
[PATCH] coroutines : Avoid a C++11ism.
The master version of the code uses a defaulted CTOR, which had
been inadvertently backported to gcc-10. Fixed thus.
gcc/cp/ChangeLog:
* coroutines.cc (struct var_nest_node): Provide a default
CTOR.
---
gcc/cp/coroutines.cc | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 9133f024434..4902d1a4c4f 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2697,7 +2697,9 @@ find_interesting_subtree (tree *expr_p, int *dosub,
void *d)
struct var_nest_node
{
- var_nest_node () = default;
+ var_nest_node ()
+: var(NULL_TREE), init(NULL_TREE),
+ prev(NULL), next(NULL), then_cl(NULL), else_cl(NULL) {}
var_nest_node (tree v, tree i, var_nest_node *p, var_nest_node *n)
: var(v), init(i), prev(p), next(n)
{
--
2.24.1
Re: [PATCH] coroutines: Correct frame capture of compiler temps [PR95591+4].
Iain Sandoe wrote:
> Jason Merrill wrote:
>
>> I notice this patch includes
>>
>> + var_nest_node () = default;
>>
>> which will break GCC 10 bootstrap with a C++98 compiler; we only
>> switched to C++11 for GCC 11.
>
> Hmm, the patch was already backported…
> … I will fix this.
>
> I missed the warning during testing.
We set -std=gnu++98 for the stage1 compiler, which does warn about this
(although there
is a lot of warning output from a bootstrap, it’s easy to miss one).
I’m mostly bootstrapping with GCC-7.5 and it appears that this does not
actually produce
any error even if one actually uses the defaulted ctor (at least in a trivial
test).
= anyway ….
OK for gcc-10?
this removes the warning from the stage #1.
thanks
Iain
==
[PATCH] coroutines : Avoid a C++11ism.
The master version of the code uses a defaulted CTOR, which had
been inadvertently backported to gcc-10. Fixed thus.
gcc/cp/ChangeLog:
* coroutines.cc (struct var_nest_node): Provide a default
CTOR.
---
gcc/cp/coroutines.cc | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
index 9133f024434..4902d1a4c4f 100644
--- a/gcc/cp/coroutines.cc
+++ b/gcc/cp/coroutines.cc
@@ -2697,7 +2697,9 @@ find_interesting_subtree (tree *expr_p, int *dosub, void
*d)
struct var_nest_node
{
- var_nest_node () = default;
+ var_nest_node ()
+: var(NULL_TREE), init(NULL_TREE),
+ prev(NULL), next(NULL), then_cl(NULL), else_cl(NULL) {}
var_nest_node (tree v, tree i, var_nest_node *p, var_nest_node *n)
: var(v), init(i), prev(p), next(n)
{
--
2.24.1
Re: [PATCH] coroutines: Correct frame capture of compiler temps [PR95591+4].
Jason Merrill wrote: I notice this patch includes + var_nest_node () = default; which will break GCC 10 bootstrap with a C++98 compiler; we only switched to C++11 for GCC 11. Hmm, the patch was already backported… … I will fix this. I missed the warning during testing. Iain
Re: [PATCH] coroutines: Correct frame capture of compiler temps [PR95591+4].
I notice this patch includes
+ var_nest_node () = default;
which will break GCC 10 bootstrap with a C++98 compiler; we only
switched to C++11 for GCC 11.
On Fri, Jul 17, 2020 at 8:57 AM Nathan Sidwell wrote:
>
> On 7/17/20 3:37 AM, Richard Biener wrote:
> > On Thu, Jul 16, 2020 at 7:39 PM Nathan Sidwell wrote:
> >>
> >> On 7/16/20 11:56 AM, Iain Sandoe wrote:
> >>> Hello folks,
> >>
> >> It's unfortunate the original plan of handling lifetime issues in a
> >> gimple pass didn't work out.
> >>
> >>> OK for master?
> >>>
> >>> OK for 10.x?
> >
> > IMHO too big of a change for 10.2 but of course fine after the
> > 10.2 release which gives it some beating-time on trunk as well.
>
> I agree. I forgot to add that after reviewing. We (FB) will apply this
> patch when updating to 10.2. Given we fell over many of the bugs, it'd
> be rude not to!
>
> >
> > Richard.
> >
> >> ok, some nits
> >>> +struct interesting
> >>> +{
> >>
> >> not a very good name. As it's not in an anonumous namespace, it has
> >> external linkage, and hence ripe for ODR breakage, if some other TU is
> >> interested about something else :) Is there a more specific name it
> >> could have?
> >>
> >>
> >>> + else if ((tmp_target_expr_p (expr)
> >>> + && !p->temps_used->contains (expr)))
> >>
> >> too many parens here.
> >>
> >> nathan
> >>
> >> --
> >> Nathan Sidwell
>
>
> --
> Nathan Sidwell
>
Re: [PATCH] coroutines: Correct frame capture of compiler temps [PR95591+4].
On 7/17/20 3:37 AM, Richard Biener wrote:
On Thu, Jul 16, 2020 at 7:39 PM Nathan Sidwell wrote:
On 7/16/20 11:56 AM, Iain Sandoe wrote:
Hello folks,
It's unfortunate the original plan of handling lifetime issues in a
gimple pass didn't work out.
OK for master?
OK for 10.x?
IMHO too big of a change for 10.2 but of course fine after the
10.2 release which gives it some beating-time on trunk as well.
I agree. I forgot to add that after reviewing. We (FB) will apply this
patch when updating to 10.2. Given we fell over many of the bugs, it'd
be rude not to!
Richard.
ok, some nits
+struct interesting
+{
not a very good name. As it's not in an anonumous namespace, it has
external linkage, and hence ripe for ODR breakage, if some other TU is
interested about something else :) Is there a more specific name it
could have?
+ else if ((tmp_target_expr_p (expr)
+ && !p->temps_used->contains (expr)))
too many parens here.
nathan
--
Nathan Sidwell
--
Nathan Sidwell
Re: [PATCH] coroutines: Correct frame capture of compiler temps [PR95591+4].
On Thu, Jul 16, 2020 at 7:39 PM Nathan Sidwell wrote:
>
> On 7/16/20 11:56 AM, Iain Sandoe wrote:
> > Hello folks,
>
> It's unfortunate the original plan of handling lifetime issues in a
> gimple pass didn't work out.
>
> > OK for master?
> >
> > OK for 10.x?
IMHO too big of a change for 10.2 but of course fine after the
10.2 release which gives it some beating-time on trunk as well.
Richard.
> ok, some nits
> > +struct interesting
> > +{
>
> not a very good name. As it's not in an anonumous namespace, it has
> external linkage, and hence ripe for ODR breakage, if some other TU is
> interested about something else :) Is there a more specific name it
> could have?
>
>
> > + else if ((tmp_target_expr_p (expr)
> > + && !p->temps_used->contains (expr)))
>
> too many parens here.
>
> nathan
>
> --
> Nathan Sidwell
Re: [PATCH] coroutines: Correct frame capture of compiler temps [PR95591+4].
Nathan Sidwell wrote:
On 7/16/20 11:56 AM, Iain Sandoe wrote:
It's unfortunate the original plan of handling lifetime issues in a
gimple pass didn't work out.
I’ve revisited this (on balance every couple of months, I suppose when it
gets tricky doing
something in the FE). However, it seems to me that the complexity is
inherent and all we
would do is to punt that from the FE into the ME.
for example, on the limited tests I made with coro suspend point lowering
delayed until post
LTO, I ran into issues with loads of pointers to the promise getting
hoisted out of loops with
suspend points. Of course, everything is probably fixable - and there’s an
existing impl that
defers the frame consruction to the ME (noting that there's a comment from
the authors there
saying that they do end up with some unnecessary frame entries).
some things are easier to analyze in tree-land and some things are def.
easier when the
code has been flattened.
There’s at least one optimisation of the size of the coroutine frame that I
think would be
much more difficult (maybe impossible, depending on what information has
been discarded)
later in the pipeline.
so .. my current estimate remains that it would be “six of one, half a
dozen of the other”.
+struct interesting
+{
not a very good name. As it's not in an anonumous namespace, it has
external linkage, and hence ripe for ODR breakage, if some other TU is
interested about something else :) Is there a more specific name it
could have?
changed to ‘coro_interesting_subtree’.
+ else if ((tmp_target_expr_p (expr)
+ && !p->temps_used->contains (expr)))
too many parens here.
thanks, fixed
pushed to master.
Iain
* for master (at least on Darwin):
we now get 100% pass on cppcoro
and 99% on folly (one expected fail, one to be analyzed).
Re: [PATCH] coroutines: Correct frame capture of compiler temps [PR95591+4].
On 7/16/20 11:56 AM, Iain Sandoe wrote:
Hello folks,
It's unfortunate the original plan of handling lifetime issues in a
gimple pass didn't work out.
OK for master?
OK for 10.x?
ok, some nits
+struct interesting
+{
not a very good name. As it's not in an anonumous namespace, it has
external linkage, and hence ripe for ODR breakage, if some other TU is
interested about something else :) Is there a more specific name it
could have?
+ else if ((tmp_target_expr_p (expr)
+ && !p->temps_used->contains (expr)))
too many parens here.
nathan
--
Nathan Sidwell
[PATCH] coroutines: Correct frame capture of compiler temps [PR95591+4].
Hello folks, This is a patch correcting a thinko on my part which turns out to have quite serious consequences for coroutines. Sorry it took longer to clean up than I expected. 1/ I am sad that this is (too) late for 10.2 .. .. my advice to distros would be to apply the patch to 10.2… I will certainly to that for Darwin. coroutines is still behind -fcoroutines on 10.x, and the code changes are confined to coroutines.cc. So .. if there’s a respin .. I would definitely put this forward to add in (and for my 0.02GBP would probably add it anyway). 2/ it’s quite a large patch (sorry). master tested on x86_64/powerpc64 Linux, x86_64-darwin also the 10.2 back-port has been tested in the same configurations against 10.2rc1. needs 608832716e27ca356ee38d14ae30b3ab525884ea which is also coro-specific. OK for master? OK for 10.x? thanks, Iain * test cases as a text attachment, probably not interesting inline. —— commit message. When a full expression contains a co_await (or co_yield), this means that it might be suspended; which would destroy temporary variables (on a stack for example). However the language guarantees that such temporaries are live until the end of the expression. In order to preserve this, we 'promote' temporaries where necessary so that they are saved in the coroutine frame (which allows them to remain live potentially until the frame is destroyed). In addition, sub-expressions that produce control flow (such as TRUTH_AND/OR_IF or COND_EXPR) must be handled specifically to ensure that await expressions are properly expanded. This patch corrects two mistakes in which we were (a) failing to promote some temporaries and (b) we we failing to sequence DTORs for the captures properly. This manifests in a number of related (but not exact duplicate) PRs. The revised code collects the actions into one place and maps all the control flow into one form - a benefit of this is that co_returns are now expanded earlier (which provides an opportunity to address PR95517 in some future patch). We replace a statement that contains await expression(s) with a bind scope that has a variable list describing the temporaries that have been 'promoted' and a statement list that contains a series of cleanup expressions for each of those. Where we encounter nested conditional expressions, these are wrapped in a try-finally block with a guard var for each sub-expression variable that needs a DTOR. The guards are all declared and initialized to false before the first conditional sub- expression. The 'finally' block contains a series of if blocks (one per guard variable) enclosing the relevant DTOR. Variables listed in a bind scope in this manner are automatically moved to a coroutine frame version by existing code (so we re-use that rather than having a separate mechanism). gcc/cp/ChangeLog: PR c++/95591 PR c++/95599 PR c++/95823 PR c++/95824 PR c++/95895 * coroutines.cc (struct coro_ret_data): Delete. (coro_maybe_expand_co_return): Delete. (co_return_expander): Delete. (expand_co_returns): Delete. (co_await_find_in_subtree): Remove unused name. (build_actor_fn): Remove unused parm, remove handling for co_return expansion. (register_await_info): Demote duplicate info message to a warning. (coro_make_frame_entry): Move closer to use site. (struct susp_frame_data): Add fields for final suspend label and a flag to indicate await expressions with initializers. (captures_temporary): Delete. (register_awaits): Remove unused code, update comments. (find_any_await): New. (tmp_target_expr_p): New. (struct interesting): New. (find_interesting_subtree): New. (struct var_nest_node): New. (flatten_await_stmt): New. (handle_nested_conditionals): New. (process_conditional): New. (replace_statement_captures): Rename to... (maybe_promote_temps): ... this. (maybe_promote_captured_temps): Delete. (analyze_expression_awaits): Check for await expressions with initializers. Simplify handling for truth-and/or-if. (expand_one_truth_if): Simplify (map cases that need expansion to COND_EXPR). (await_statement_walker): Handle CO_RETURN_EXPR. Simplify the handling for truth-and/or-if expressions. (register_local_var_uses): Ensure that we create names in the implementation namespace. (morph_fn_to_coro): Add final suspend label to suspend frame callback data and remove it from the build_actor_fn call. gcc/testsuite/ChangeLog: PR c++/95591 PR c++/95599 PR c++/95823 PR c++/95824 PR c++/95895 * g++.dg/coroutines/pr95591.C: New test. * g++.dg/coroutines/pr95599.C: New test. * g++.dg/coroutines/pr95823.C: New test. * g++.dg/coroutine
