On 5/7/24 01:35, Nathaniel Shead wrote:
On Thu, May 02, 2024 at 01:53:44PM -0400, Jason Merrill wrote:
On 5/2/24 10:40, Patrick Palka wrote:
On Thu, 2 May 2024, Nathaniel Shead wrote:
Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk/14.2?
Another alternative would be to stream such !DECL_NAME temporaries with
a merge key of MK_unique rather than attempting to find the matching
(nonexistant) field of the class context.
Both approaches sound good to me, hard to say which one is preferable..
The handling of function-scope vs class-scope temporaries seems to start
diverging in:
@@ -8861,28 +8861,6 @@ trees_out::decl_node (tree decl, walk_kind ref)
return false;
}
! tree ctx = CP_DECL_CONTEXT (decl);
! depset *dep = NULL;
! if (streaming_p ())
! dep = dep_hash->find_dependency (decl);
! else if (TREE_CODE (ctx) != FUNCTION_DECL
! || TREE_CODE (decl) == TEMPLATE_DECL
! || DECL_IMPLICIT_TYPEDEF_P (decl)
! || (DECL_LANG_SPECIFIC (decl)
! && DECL_MODULE_IMPORT_P (decl)))
! {
! auto kind = (TREE_CODE (decl) == NAMESPACE_DECL
! && !DECL_NAMESPACE_ALIAS (decl)
! ? depset::EK_NAMESPACE : depset::EK_DECL);
! dep = dep_hash->add_dependency (decl, kind);
! }
!
! if (!dep)
! {
! /* Some internal entity of context. Do by value. */
! decl_value (decl, NULL);
! return false;
! }
if (dep->get_entity_kind () == depset::EK_REDIRECT)
{
where for a class-scope temporary we add a dependency for it, stream
it by reference, and then stream it by value separately, which seems
unnecessary.
So if we decide to keep the create_temporary_var change, we probably
would want to unify this code path's handling of temporaries (i.e.
don't add_dependency a temporary regardless of its context).
If we decide your partially revert the create_temporary_var change,
your patch LGTM.
Streaming by value sounds right, but as noted an important difference
between reference temps and others is DECL_NAME. Perhaps the code Patrick
quotes could look at that as well as the context?
Jason
With my patch we would no longer go through the code that Patrick quotes
for class-scope temporaries that I can see; we would instead first hit
the following code in 'tree_node':
if (DECL_P (t))
{
if (DECL_TEMPLATE_PARM_P (t))
{
tpl_parm_value (t);
goto done;
}
if (!DECL_CONTEXT (t))
{
/* There are a few cases of decls with no context. We'll write
these by value, but first assert they are cases we expect. */
gcc_checking_assert (ref == WK_normal);
switch (TREE_CODE (t))
{
default: gcc_unreachable ();
case LABEL_DECL:
/* CASE_LABEL_EXPRs contain uncontexted LABEL_DECLs. */
gcc_checking_assert (!DECL_NAME (t));
break;
case VAR_DECL:
/* AGGR_INIT_EXPRs cons up anonymous uncontexted VAR_DECLs. */
gcc_checking_assert (!DECL_NAME (t)
&& DECL_ARTIFICIAL (t));
break;
case PARM_DECL:
/* REQUIRES_EXPRs have a tree list of uncontexted
PARM_DECLS. It'd be nice if they had a
distinguishing flag to double check. */
break;
}
goto by_value;
}
}
skip_normal:
if (DECL_P (t) && !decl_node (t, ref))
goto done;
/* Otherwise by value */
by_value:
tree_value (t);
I think modifying what Patrick pointed out should only be necessary if
we maintain these nameless temporaries as having a class context; for
clarity, is that the direction you'd prefer me to go in to solve this?
I was thinking in this code that it seems fragile to require null
DECL_CONTEXT to identify something produced by
create_temporary_var/build_local_temp (which should really be merged).
We could even use is_local_temp instead of checking particular qualities
directly.
But your patch is OK; please just add a comment to the
make_temporary_var_for_ref_to_temp change indicating that you're setting
DECL_CONTEXT to make the variable mergeable.
Thanks,
Jason
