Fix type merging deficiency during WPA
diff mbox

Message ID CAFiYyc3mF2iHVUhyLWHsSXC1_TuJxo_7_=ZbnRNZXvB0sF8pYQ@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener July 6, 2016, 2 p.m. UTC
On Wed, Jul 6, 2016 at 12:44 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Jul 6, 2016 at 12:21 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jul 6, 2016 at 11:33 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>>> I see.  I think the solution is to perform cgraph/varpool merging
>>>> before attempting to read in
>>>> the global decl stream.  IIRC Micha had (old) patches for this.
>>>
>>> How can you merge varpool nodes if you haven't merged types?
>
> You merge them just in the way the linker instructs you via the
> resolution table.
>
>>>> But I wonder why we don't tree-merge 'n' here (from my C example) and
>>>> thus figure
>>>> that the type domain of x is equal?  Or is it that 'n' and 'x' are in
>>>> the same SCC (they
>>>> referece each other in some way)?  In this case the bug would be that we
>>>> fail to treat them equal optimistically.  That said, I don't see how
>>>> TYPE_CANONICAL computation is relevant - what is relevant is the failure to
>>>> merge the two types.
>>>> In debugging this I'd start to see if the hashes are not equal or if
>>>> they are equal
>>>> at which node we consider them to differ.
>>>
>>> We just have 2 different DECLs with different DECL_UIDs, the definition from a
>>> compilation unit and a reference from another compilation unit, so the hashes
>>> naturally differ too.  What's supposed to have them reconciled at this point?
>>
>> I am talking about tree/SCC merging which happily merges global decls as well.
>> It uses custom hashing (see lto-streamer-out.c:hash_tree) which doesn't hash
>> DECL_UID (obviously).  This merging process should be optimistic for all nodes
>> in the same SCC as well.
>>
>> That said, I expect the types to be tree merged and wonder why they are not.
>>
>> If they were merged they'd obviously share TYPE_CANONICAL because there
>> would be only one type to compute TYPE_CANONICAL for.
>
> It probably boils down to one unit refering to the DECL with DECL_EXTERNAL set
> and one to the DECL with TREE_STATIC set?  This would mean that
> hashing/comparing
> should be set up to "merge" those but the merging ultimatively rejected by some
> toplevel logic (so we get users merged).  But as said, early symtab
> merging would
> fix this as well.

So sth like


plus the magic new function same_decls which would check if the trees are
var/function decls with the same assembler name. The function can't use
the cgraph as that is not yet read in unfortunately - I still think we should
do that so we can see the prevailing nodes early.

Richard.

>
> Richard.
>
>> Richard.
>>
>>> The TYPE_CANONICAL computation is relevant because, with GCC 6, the criterion
>>> for compatibility of pointer types is the alias set, which is based on the
>>> TYPE_CANONICAL of the pointed-to type, so we fail to merge pointer types
>>> because warn_type_compatibility_p returns non-zero if TYPE_CANONICAL differs.
>>>
>>> --
>>> Eric Botcazou

Patch
diff mbox

Index: gcc/lto-streamer-out.c
===================================================================
--- gcc/lto-streamer-out.c      (revision 238039)
+++ gcc/lto-streamer-out.c      (working copy)
@@ -996,7 +996,7 @@  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));
+  //hstate.add_flag (TREE_STATIC (t));
   hstate.add_flag (TREE_PROTECTED (t));
   hstate.add_flag (TREE_DEPRECATED (t));
   if (code != TREE_BINFO)
@@ -1050,7 +1050,7 @@  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));
+      //hstate.add_flag (DECL_EXTERNAL (t));
       hstate.add_flag (DECL_GIMPLE_REG_P (t));
       hstate.commit_flag ();
       hstate.add_int (DECL_ALIGN (t));
Index: gcc/lto/lto.c
===================================================================
--- gcc/lto/lto.c       (revision 238039)
+++ gcc/lto/lto.c       (working copy)
@@ -1263,7 +1263,8 @@  compare_tree_sccs_1 (tree t1, tree t2, t
     tree t1_ = (E1), t2_ = (E2); \
     if (t1_ != t2_ \
        && (!t1_ || !t2_ \
-           || !TREE_VISITED (t2_) \
+           || (!TREE_VISITED (t2_) \
+               && !same_decls (t1_, t2_)) \
            || (!TREE_ASM_WRITTEN (t2_) \
                && !compare_tree_sccs_1 (t1_, t2_, map)))) \
       return false; \