Patchwork [GOOGLE] Fixup varpool references after LIPO linking

login
register
mail settings
Submitter Teresa Johnson
Date Aug. 28, 2014, 8:29 p.m.
Message ID <CAAe5K+WrDHbQxc1diCv1x85c0LT4YCg2UGnDajA=E64jZ9pS8w@mail.gmail.com>
Download mbox | patch
Permalink /patch/384014/
State New
Headers show

Comments

Teresa Johnson - Aug. 28, 2014, 8:29 p.m.
This patch fixes up varpool nodes after LIPO linking as we were doing
for cgraph node references. While, here I made some fixes to the
cgraph fixup as well (mark address taken as appropriate) and removed
old references. The latter exposed an issue with resolved cgraph nodes
getting eliminated when they were only referenced from vtables and the
LIPO linking selected an external copy as the resolved node. Addressed
this by forcing the LIPO linking to prefer the non-external copy.

Passes regression testing, internal benchmark testing in progress. Ok
for google/4_9 if that succeeds?

Teresa

2014-08-28  Teresa Johnson  <tejohnson@google.com>

        Google ref b/17038802.
        * l-ipo.c (resolve_cgraph_node): Pick non-external node.
        (fixup_reference_list): Fixup varpool references, remove old
        references, mark cgraph nodes as address taken as needed.
Teresa Johnson - Aug. 28, 2014, 8:46 p.m.
On Thu, Aug 28, 2014 at 1:40 PM, Xinliang David Li <davidxl@google.com> wrote:
>
> Do you know why the previous check is not enough ?
>
> cgraph_can_remove_if_no_direct_calls_and_refs_p (struct cgraph_node *node)

This will return true for the external node, but it also returns true
for the non-external node. The non-external node is a COMDAT, as as
the comments in that routine indicate, COMDATs can be removed even if
they are externally_visible.

Teresa

>
> David
>
>
> On Thu, Aug 28, 2014 at 1:29 PM, Teresa Johnson <tejohnson@google.com>
> wrote:
>>
>> This patch fixes up varpool nodes after LIPO linking as we were doing
>> for cgraph node references. While, here I made some fixes to the
>> cgraph fixup as well (mark address taken as appropriate) and removed
>> old references. The latter exposed an issue with resolved cgraph nodes
>> getting eliminated when they were only referenced from vtables and the
>> LIPO linking selected an external copy as the resolved node. Addressed
>> this by forcing the LIPO linking to prefer the non-external copy.
>>
>> Passes regression testing, internal benchmark testing in progress. Ok
>> for google/4_9 if that succeeds?
>>
>> Teresa
>>
>> 2014-08-28  Teresa Johnson  <tejohnson@google.com>
>>
>>         Google ref b/17038802.
>>         * l-ipo.c (resolve_cgraph_node): Pick non-external node.
>>         (fixup_reference_list): Fixup varpool references, remove old
>>         references, mark cgraph nodes as address taken as needed.
>>
>> Index: l-ipo.c
>> ===================================================================
>> --- l-ipo.c     (revision 213975)
>> +++ l-ipo.c     (working copy)
>> @@ -1564,6 +1564,15 @@ resolve_cgraph_node (struct cgraph_sym **slot, str
>>            (*slot)->rep_decl = decl2;
>>            return;
>>          }
>> +      /* Similarly, pick the non-external symbol, since external
>> +         symbols may be eliminated by symtab_remove_unreachable_nodes
>> +         after ipa inlining (see process_references).  */
>> +      if (DECL_EXTERNAL (decl1) && !DECL_EXTERNAL (decl2))
>> +        {
>> +          (*slot)->rep_node = node;
>> +          (*slot)->rep_decl = decl2;
>> +          return;
>> +        }
>>
>>        has_prof1 = has_profile_info (decl1);
>>        bool is_aux1 = cgraph_is_auxiliary (decl1);
>> @@ -2304,31 +2313,44 @@ fixup_reference_list (struct varpool_node *node)
>>    int i;
>>    struct ipa_ref *ref;
>>    struct ipa_ref_list *list = &node->ref_list;
>> -  vec<cgraph_node_ptr> new_refered;
>> +  vec<symtab_node *> new_refered;
>>    vec<int> new_refered_type;
>> -  struct cgraph_node *c;
>> +  struct symtab_node *sym_node;
>>    enum ipa_ref_use use_type = IPA_REF_LOAD;
>>
>>    new_refered.create (10);
>>    new_refered_type.create (10);
>>    for (i = 0; ipa_ref_list_reference_iterate (list, i, ref); i++)
>>      {
>> -      if (!is_a <cgraph_node> (ref->referred))
>> -        continue;
>> -
>> -      struct cgraph_node *cnode = ipa_ref_node (ref);
>> -      struct cgraph_node *r_cnode
>> -        = cgraph_lipo_get_resolved_node (cnode->decl);
>> -      if (r_cnode != cnode)
>> +      if (is_a <cgraph_node> (ref->referred))
>>          {
>> +          struct cgraph_node *cnode = ipa_ref_node (ref);
>> +          struct cgraph_node *r_cnode
>> +            = cgraph_lipo_get_resolved_node (cnode->decl);
>>            new_refered.safe_push (r_cnode);
>>            use_type = ref->use;
>>            new_refered_type.safe_push ((int) use_type);
>> +          gcc_assert (use_type != IPA_REF_ADDR
>> +                      || cnode->global.inlined_to
>> +                      || cnode->address_taken);
>> +          if (use_type == IPA_REF_ADDR)
>> +            cgraph_mark_address_taken_node (r_cnode);
>>          }
>> +      else if (is_a <varpool_node> (ref->referred))
>> +        {
>> +          struct varpool_node *var = ipa_ref_varpool_node (ref);
>> +          struct varpool_node *r_var = real_varpool_node (var->decl);
>> +          new_refered.safe_push (r_var);
>> +          use_type = ref->use;
>> +          new_refered_type.safe_push ((int) use_type);
>> +        }
>> +      else
>> +        gcc_assert (false);
>>      }
>> -  for (i = 0; new_refered.iterate (i, &c); ++i)
>> +  ipa_remove_all_references (&node->ref_list);
>> +  for (i = 0; new_refered.iterate (i, &sym_node); ++i)
>>      {
>> -      ipa_record_reference (node, c,
>> +      ipa_record_reference (node, sym_node,
>>                              (enum ipa_ref_use) new_refered_type[i],
>> NULL);
>>      }
>>  }
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>
>
Xinliang David Li - Aug. 28, 2014, 8:52 p.m.
ok. The patch is fine.

On Thu, Aug 28, 2014 at 1:46 PM, Teresa Johnson <tejohnson@google.com> wrote:
> On Thu, Aug 28, 2014 at 1:40 PM, Xinliang David Li <davidxl@google.com> wrote:
>>
>> Do you know why the previous check is not enough ?
>>
>> cgraph_can_remove_if_no_direct_calls_and_refs_p (struct cgraph_node *node)
>
> This will return true for the external node, but it also returns true
> for the non-external node. The non-external node is a COMDAT, as as
> the comments in that routine indicate, COMDATs can be removed even if
> they are externally_visible.
>
> Teresa
>
>>
>> David
>>
>>
>> On Thu, Aug 28, 2014 at 1:29 PM, Teresa Johnson <tejohnson@google.com>
>> wrote:
>>>
>>> This patch fixes up varpool nodes after LIPO linking as we were doing
>>> for cgraph node references. While, here I made some fixes to the
>>> cgraph fixup as well (mark address taken as appropriate) and removed
>>> old references. The latter exposed an issue with resolved cgraph nodes
>>> getting eliminated when they were only referenced from vtables and the
>>> LIPO linking selected an external copy as the resolved node. Addressed
>>> this by forcing the LIPO linking to prefer the non-external copy.
>>>
>>> Passes regression testing, internal benchmark testing in progress. Ok
>>> for google/4_9 if that succeeds?
>>>
>>> Teresa
>>>
>>> 2014-08-28  Teresa Johnson  <tejohnson@google.com>
>>>
>>>         Google ref b/17038802.
>>>         * l-ipo.c (resolve_cgraph_node): Pick non-external node.
>>>         (fixup_reference_list): Fixup varpool references, remove old
>>>         references, mark cgraph nodes as address taken as needed.
>>>
>>> Index: l-ipo.c
>>> ===================================================================
>>> --- l-ipo.c     (revision 213975)
>>> +++ l-ipo.c     (working copy)
>>> @@ -1564,6 +1564,15 @@ resolve_cgraph_node (struct cgraph_sym **slot, str
>>>            (*slot)->rep_decl = decl2;
>>>            return;
>>>          }
>>> +      /* Similarly, pick the non-external symbol, since external
>>> +         symbols may be eliminated by symtab_remove_unreachable_nodes
>>> +         after ipa inlining (see process_references).  */
>>> +      if (DECL_EXTERNAL (decl1) && !DECL_EXTERNAL (decl2))
>>> +        {
>>> +          (*slot)->rep_node = node;
>>> +          (*slot)->rep_decl = decl2;
>>> +          return;
>>> +        }
>>>
>>>        has_prof1 = has_profile_info (decl1);
>>>        bool is_aux1 = cgraph_is_auxiliary (decl1);
>>> @@ -2304,31 +2313,44 @@ fixup_reference_list (struct varpool_node *node)
>>>    int i;
>>>    struct ipa_ref *ref;
>>>    struct ipa_ref_list *list = &node->ref_list;
>>> -  vec<cgraph_node_ptr> new_refered;
>>> +  vec<symtab_node *> new_refered;
>>>    vec<int> new_refered_type;
>>> -  struct cgraph_node *c;
>>> +  struct symtab_node *sym_node;
>>>    enum ipa_ref_use use_type = IPA_REF_LOAD;
>>>
>>>    new_refered.create (10);
>>>    new_refered_type.create (10);
>>>    for (i = 0; ipa_ref_list_reference_iterate (list, i, ref); i++)
>>>      {
>>> -      if (!is_a <cgraph_node> (ref->referred))
>>> -        continue;
>>> -
>>> -      struct cgraph_node *cnode = ipa_ref_node (ref);
>>> -      struct cgraph_node *r_cnode
>>> -        = cgraph_lipo_get_resolved_node (cnode->decl);
>>> -      if (r_cnode != cnode)
>>> +      if (is_a <cgraph_node> (ref->referred))
>>>          {
>>> +          struct cgraph_node *cnode = ipa_ref_node (ref);
>>> +          struct cgraph_node *r_cnode
>>> +            = cgraph_lipo_get_resolved_node (cnode->decl);
>>>            new_refered.safe_push (r_cnode);
>>>            use_type = ref->use;
>>>            new_refered_type.safe_push ((int) use_type);
>>> +          gcc_assert (use_type != IPA_REF_ADDR
>>> +                      || cnode->global.inlined_to
>>> +                      || cnode->address_taken);
>>> +          if (use_type == IPA_REF_ADDR)
>>> +            cgraph_mark_address_taken_node (r_cnode);
>>>          }
>>> +      else if (is_a <varpool_node> (ref->referred))
>>> +        {
>>> +          struct varpool_node *var = ipa_ref_varpool_node (ref);
>>> +          struct varpool_node *r_var = real_varpool_node (var->decl);
>>> +          new_refered.safe_push (r_var);
>>> +          use_type = ref->use;
>>> +          new_refered_type.safe_push ((int) use_type);
>>> +        }
>>> +      else
>>> +        gcc_assert (false);
>>>      }
>>> -  for (i = 0; new_refered.iterate (i, &c); ++i)
>>> +  ipa_remove_all_references (&node->ref_list);
>>> +  for (i = 0; new_refered.iterate (i, &sym_node); ++i)
>>>      {
>>> -      ipa_record_reference (node, c,
>>> +      ipa_record_reference (node, sym_node,
>>>                              (enum ipa_ref_use) new_refered_type[i],
>>> NULL);
>>>      }
>>>  }
>>>
>>>
>>> --
>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>>
>>
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

Patch

Index: l-ipo.c
===================================================================
--- l-ipo.c     (revision 213975)
+++ l-ipo.c     (working copy)
@@ -1564,6 +1564,15 @@  resolve_cgraph_node (struct cgraph_sym **slot, str
           (*slot)->rep_decl = decl2;
           return;
         }
+      /* Similarly, pick the non-external symbol, since external
+         symbols may be eliminated by symtab_remove_unreachable_nodes
+         after ipa inlining (see process_references).  */
+      if (DECL_EXTERNAL (decl1) && !DECL_EXTERNAL (decl2))
+        {
+          (*slot)->rep_node = node;
+          (*slot)->rep_decl = decl2;
+          return;
+        }

       has_prof1 = has_profile_info (decl1);
       bool is_aux1 = cgraph_is_auxiliary (decl1);
@@ -2304,31 +2313,44 @@  fixup_reference_list (struct varpool_node *node)
   int i;
   struct ipa_ref *ref;
   struct ipa_ref_list *list = &node->ref_list;
-  vec<cgraph_node_ptr> new_refered;
+  vec<symtab_node *> new_refered;
   vec<int> new_refered_type;
-  struct cgraph_node *c;
+  struct symtab_node *sym_node;
   enum ipa_ref_use use_type = IPA_REF_LOAD;

   new_refered.create (10);
   new_refered_type.create (10);
   for (i = 0; ipa_ref_list_reference_iterate (list, i, ref); i++)
     {
-      if (!is_a <cgraph_node> (ref->referred))
-        continue;
-
-      struct cgraph_node *cnode = ipa_ref_node (ref);
-      struct cgraph_node *r_cnode
-        = cgraph_lipo_get_resolved_node (cnode->decl);
-      if (r_cnode != cnode)
+      if (is_a <cgraph_node> (ref->referred))
         {
+          struct cgraph_node *cnode = ipa_ref_node (ref);
+          struct cgraph_node *r_cnode
+            = cgraph_lipo_get_resolved_node (cnode->decl);
           new_refered.safe_push (r_cnode);
           use_type = ref->use;
           new_refered_type.safe_push ((int) use_type);
+          gcc_assert (use_type != IPA_REF_ADDR
+                      || cnode->global.inlined_to
+                      || cnode->address_taken);
+          if (use_type == IPA_REF_ADDR)
+            cgraph_mark_address_taken_node (r_cnode);
         }
+      else if (is_a <varpool_node> (ref->referred))
+        {
+          struct varpool_node *var = ipa_ref_varpool_node (ref);
+          struct varpool_node *r_var = real_varpool_node (var->decl);
+          new_refered.safe_push (r_var);
+          use_type = ref->use;
+          new_refered_type.safe_push ((int) use_type);
+        }
+      else
+        gcc_assert (false);
     }
-  for (i = 0; new_refered.iterate (i, &c); ++i)
+  ipa_remove_all_references (&node->ref_list);
+  for (i = 0; new_refered.iterate (i, &sym_node); ++i)
     {
-      ipa_record_reference (node, c,
+      ipa_record_reference (node, sym_node,
                             (enum ipa_ref_use) new_refered_type[i], NULL);
     }
 }