Re: [patch] Fix type merging deficiency during WPA

[Richard Biener <richard dot guenther at gmail dot com>]

On Mon, Jul 11, 2016 at 5:14 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> Sorry for jumping in late, only now I had chance to read through the whole discussion.
> I was looking into similar problem some time ago.
>
>> Index: lto-streamer-out.c
>> ===================================================================
>> --- lto-streamer-out.c        (revision 238156)
>> +++ lto-streamer-out.c        (working copy)
>> @@ -996,7 +996,9 @@ hash_tree (struct streamer_tree_cache_d
>>    else
>>      hstate.add_flag (TREE_NO_WARNING (t));
>>    hstate.add_flag (TREE_NOTHROW (t));
>> -  hstate.add_flag (TREE_STATIC (t));
>> +  /* We want to unify DECL nodes in pointer fields of global types.  */
>> +  if (!(VAR_OR_FUNCTION_DECL_P (t)))
>> +    hstate.add_flag (TREE_STATIC (t));
>>    hstate.add_flag (TREE_PROTECTED (t));
>>    hstate.add_flag (TREE_DEPRECATED (t));
>>    if (code != TREE_BINFO)
>> @@ -1050,7 +1052,9 @@ hash_tree (struct streamer_tree_cache_d
>>        hstate.add_flag (DECL_ARTIFICIAL (t));
>>        hstate.add_flag (DECL_USER_ALIGN (t));
>>        hstate.add_flag (DECL_PRESERVE_P (t));
>> -      hstate.add_flag (DECL_EXTERNAL (t));
>> +      /* We want to unify DECL nodes in pointer fields of global types.  */
>> +      if (!(VAR_OR_FUNCTION_DECL_P (t)))
>> +     hstate.add_flag (DECL_EXTERNAL (t));
>
> It is fine to merge decls across static and external flags, but I am not sure this is
> a safe solution to the problem. In C it is perfectly normal to have one decl more specified
> or with different attributes.  Like:
>
> extern int a __attribute__ ((warning("bar"));
>
> int a=7 __attribute__ ((warning("foo")));
>
> which must prevent merging otherwise the warnings will go out wrong way. In GCC
> 6 timeframe we decided to live with duplicated declarations and added support
> for syntactic aliases. Tree merging should be optional WRT correctness, so I think
> bug is in the canonical type calculation:
>
>   /* For array types hash the domain bounds and the string flag.  */
>   if (TREE_CODE (type) == ARRAY_TYPE && TYPE_DOMAIN (type))
>     {
>       hstate.add_int (TYPE_STRING_FLAG (type));
>       /* OMP lowering can introduce error_mark_node in place of
>          random local decls in types.  */
>       if (TYPE_MIN_VALUE (TYPE_DOMAIN (type)) != error_mark_node)
>         inchash::add_expr (TYPE_MIN_VALUE (TYPE_DOMAIN (type)), hstate);
>       if (TYPE_MAX_VALUE (TYPE_DOMAIN (type)) != error_mark_node)
>         inchash::add_expr (TYPE_MAX_VALUE (TYPE_DOMAIN (type)), hstate);
>     }
>
> which boils down to:
>
>       if (tclass == tcc_declaration)
>         {
>           /* DECL's have a unique ID */
>           hstate.add_wide_int (DECL_UID (t));
>         }
>
> (in asd_expr)

Well, I think the patch wouldn't merge those it simply treats them the same
for references to outside of a tree SCC.  So it's kind of a hack...

I think the real fix is to apply the linker resolution to the cgraph early
(which means reading the cgraph before reading global decls), keeping
a decl-ref to symtab node hash for the purpose of tree merging.

> It is bit ugly, but I think for static/external/public decls we need to use
> assembler name here (as we can't rely on symtab to be around all the time)
> which will result in unstability WRT symbol renaming and also give false
> positives for static symbols. But even for static symbols it is not guaranteed
> they are not duplicated.  It may be possible to use combination of assembler
> name and translation unit that is available from symtab node, but not all decls
> will have it defined.

Yeah, but assembler name is in the cgraph which is not read in yet...
(so I wonder how same_global_decl_p in the patch works ...).  Oh, it isn't
yet in the symbol table...

Still I guess it should only compare if DECL_ASSEMBLER_NAME_SET_P.

And I think the patch (without the LTO_SET_PREVAIL bits) is sensible.

Richard.

>
> Honza