Message ID | 20220428191704.89750-1-iain@sandoe.co.uk |
---|---|
State | New |
Headers | show |
Series | c++, coroutines: Partial reversion of r12-8308-g15a176a833f23e [PR105426]. | expand |
On Thu, Apr 28, 2022 at 08:17:04PM +0100, Iain Sandoe wrote: > Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> > > PR c++/105426 > > gcc/cp/ChangeLog: > > * coroutines.cc (register_local_var_uses): Allow promotion of unnamed > temporaries to coroutine frame copies. > --- > gcc/cp/coroutines.cc | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > index 551ddc9cc41..2e393b2cddc 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -3973,6 +3973,9 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) > else if (lvname != NULL_TREE) > buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname), > lvd->nest_depth, lvd->bind_indx); > + else > + buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth, > + lvd->bind_indx); > /* TODO: Figure out if we should build a local type that has any > excess alignment or size from the original decl. */ > if (buf) This isn't going to play well with -fcompare-debug. We don't guarantee same DECL_UID values between -g and -g0, just that when two decls are created with both -g and -g0 they compare the same, but with -g the gap in between them could be larger. So names that include DECL_UID will be often different between -g and -g0. Can't the FIELD_DECL be instead nameless? Say change coro_make_frame_entry to do tree id = name ? get_identifier (name) : NULL_TREE; instead of tree id = get_identifier (name); ? Jakub
> On 28 Apr 2022, at 20:26, Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Apr 28, 2022 at 08:17:04PM +0100, Iain Sandoe wrote: >> Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> >> >> PR c++/105426 >> >> gcc/cp/ChangeLog: >> >> * coroutines.cc (register_local_var_uses): Allow promotion of unnamed >> temporaries to coroutine frame copies. >> --- >> gcc/cp/coroutines.cc | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >> index 551ddc9cc41..2e393b2cddc 100644 >> --- a/gcc/cp/coroutines.cc >> +++ b/gcc/cp/coroutines.cc >> @@ -3973,6 +3973,9 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) >> else if (lvname != NULL_TREE) >> buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname), >> lvd->nest_depth, lvd->bind_indx); >> + else >> + buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth, >> + lvd->bind_indx); >> /* TODO: Figure out if we should build a local type that has any >> excess alignment or size from the original decl. */ >> if (buf) > > This isn't going to play well with -fcompare-debug. > We don't guarantee same DECL_UID values between -g and -g0, just that > when two decls are created with both -g and -g0 they compare the same, > but with -g the gap in between them could be larger. > So names that include DECL_UID will be often different between -g and -g0. that’s somewhat unfortunate (having the UIDs in the frame and the gimple is quite helpful to debugging). I guess I was not expecting a difference this early in the FE (we are before initial folding / constexpr stuff). FWIW, I ran the coroutines and coroutines torture testsuites with -fcompare-debug. There are 5 fails, and those are unaffected by whether the field is made nameless as suggested below (so something else to investigate). > Can't the FIELD_DECL be instead nameless? Say change coro_make_frame_entry > to do > tree id = name ? get_identifier (name) : NULL_TREE; > instead of > tree id = get_identifier (name); Unfortunately, that does not work - although I cannot see anything in the coroutines code that would cause it to fail - perhaps something is not playing nicely with DECL_VALUE_EXPRs on unnamed temps? how about the following, which uniques the names by bind scope, scope nest and then sequence within that? Iain diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index 2e393b2cddc..1d886b31c77 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -3913,6 +3913,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) if (TREE_CODE (*stmt) == BIND_EXPR) { tree lvar; + unsigned serial = 0; for (lvar = BIND_EXPR_VARS (*stmt); lvar != NULL; lvar = DECL_CHAIN (lvar)) { @@ -3974,16 +3975,14 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname), lvd->nest_depth, lvd->bind_indx); else - buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth, - lvd->bind_indx); + buf = xasprintf ("_D%u_%u_%u", lvd->nest_depth, lvd->bind_indx, + serial++); + /* TODO: Figure out if we should build a local type that has any excess alignment or size from the original decl. */ - if (buf) - { - local_var.field_id = coro_make_frame_entry (lvd->field_list, buf, - lvtype, lvd->loc); - free (buf); - } + local_var.field_id = coro_make_frame_entry (lvd->field_list, buf, + lvtype, lvd->loc); + free (buf); /* We don't walk any of the local var sub-trees, they won't contain any bind exprs. */ }
On Thu, Apr 28, 2022 at 11:22:45PM +0100, Iain Sandoe wrote: > how about the following, which uniques the names by bind scope, scope nest and then > sequence within that? That LGTM. > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -3913,6 +3913,7 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) > if (TREE_CODE (*stmt) == BIND_EXPR) > { > tree lvar; > + unsigned serial = 0; > for (lvar = BIND_EXPR_VARS (*stmt); lvar != NULL; > lvar = DECL_CHAIN (lvar)) > { > @@ -3974,16 +3975,14 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) > buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname), > lvd->nest_depth, lvd->bind_indx); > else > - buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth, > - lvd->bind_indx); > + buf = xasprintf ("_D%u_%u_%u", lvd->nest_depth, lvd->bind_indx, > + serial++); > + > /* TODO: Figure out if we should build a local type that has any > excess alignment or size from the original decl. */ > - if (buf) > - { > - local_var.field_id = coro_make_frame_entry (lvd->field_list, buf, > - lvtype, lvd->loc); > - free (buf); > - } > + local_var.field_id = coro_make_frame_entry (lvd->field_list, buf, > + lvtype, lvd->loc); > + free (buf); > /* We don't walk any of the local var sub-trees, they won't contain > any bind exprs. */ > } > > > Jakub
diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index 551ddc9cc41..2e393b2cddc 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -3973,6 +3973,9 @@ register_local_var_uses (tree *stmt, int *do_subtree, void *d) else if (lvname != NULL_TREE) buf = xasprintf ("%s_%u_%u", IDENTIFIER_POINTER (lvname), lvd->nest_depth, lvd->bind_indx); + else + buf = xasprintf ("_D%u_%u_%u", DECL_UID (lvar), lvd->nest_depth, + lvd->bind_indx); /* TODO: Figure out if we should build a local type that has any excess alignment or size from the original decl. */ if (buf)
The changes to fix PR 105287 included a tightening of the constraints on which variables are promoted to frame copies. This has exposed that we are failing to name some variables that should be promoted. The long-term fix is to address the cases where the naming has been missed, but for the short-term (and for the GCC-12 branch) backing out the additional constraint is proposed. This makes the fix for PR 105287 more conservative. tested on x86_64-darwin, with the reproducer from the PR which now produces the correct output for both optimised and unoptimised code. OK for master? OK for GCC-12? thanks, Iain Signed-off-by: Iain Sandoe <iain@sandoe.co.uk> PR c++/105426 gcc/cp/ChangeLog: * coroutines.cc (register_local_var_uses): Allow promotion of unnamed temporaries to coroutine frame copies. --- gcc/cp/coroutines.cc | 3 +++ 1 file changed, 3 insertions(+)