Re: [PATCH] c++/modules: Stream unmergeable temporaries by value again [PR114856]

[Jason Merrill <jason@redhat.com>]

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