Patchwork [google] fix global vars incorrectly marked as read-only in LIPO mode (issue4798045)

login
register
mail settings
Submitter Rong Xu
Date July 21, 2011, 8:45 p.m.
Message ID <CAF1bQ=TKA=f6SEPia-ejeeNVac-nRqNpLZHf+9Qx-kVHarUBxQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/106160/
State New
Headers show

Comments

Rong Xu - July 21, 2011, 8:45 p.m.
Please review the new patch attached to this email.

thanks,

-Rong

On Thu, Jul 21, 2011 at 12:44 PM, Rong Xu <xur@google.com> wrote:
> wait a second. this still won't work if we disable the whole-program
> pass (like my original change, the visibility analysis change won't
> kick in). we also need to change varpool_node_link() to merge the
> local attribute.
>
> -Rong
>
> On Thu, Jul 21, 2011 at 12:35 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Thu, Jul 21, 2011 at 12:27 PM, Rong Xu <xur@google.com> wrote:
>>> this is a good point. ipa_discover_readonly_nonaddressable_vars() is
>>> called in two passes. whole-program (whole program visibility
>>> analysis) and static-var. The one in whole-program is ok here as it is
>>>  bundled together with the analysis. the invocation in static-var can
>>> go wrong.
>>>
>>> should we add a check for COMDAT flag also? like
>>
>> Probably not -- only non referenced comdat vars will be marked as not needed.
>>
>> David
>>
>>> if ((vnode->needed || (L_IPO_COMP_MODE && DECL_COMDAT (node->decl)))
>>>    && varpool_node_externally_visible_p) (
>>>
>>> Thanks,
>>>
>>> -Rong
>>> On Thu, Jul 21, 2011 at 11:59 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Thu, Jul 21, 2011 at 11:32 AM, Rong Xu <xur@google.com> wrote:
>>>>> On Thu, Jul 21, 2011 at 11:09 AM,  <davidxl@google.com> wrote:
>>>>>>
>>>>>> http://codereview.appspot.com/4798045/diff/1/ipa.c
>>>>>> File ipa.c (right):
>>>>>>
>>>>>> http://codereview.appspot.com/4798045/diff/1/ipa.c#newcode1034
>>>>>> ipa.c:1034: {
>>>>>> Has varpool node linking happened at this point? If not, the new code
>>>>>> here is not excersised.
>>>>>
>>>>> This functions is called in multiple places: local pass and
>>>>> whole_program pass. varpool_node_link is b/w these two passes. the
>>>>> varpool node attribute is set before the use in
>>>>> ipa_discover_readonly_nonaddressable_vars()
>>>>
>>>> So the code relies on the second visibility pass to get things right
>>>> -- if that pass is disabled things can go wrong (if the default
>>>> visibility value when vnode is created is true, LIPO mode will get
>>>> pessemitic result; if the default is false, LIPO mode will still get
>>>> wrong result if only local visiblity pass is run).
>>>>
>>>> A better fix might be simply do not check 'needed' bit for comdat
>>>> varnode in LIPO mode and always run the visibiity checker:
>>>>
>>>> if ((vnode->needed || L_IPO_COMP_MODE)
>>>>   && varpool_node_externally_visible_p (...
>>>>  )
>>>>
>>>>
>>>> David
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> http://codereview.appspot.com/4798045/diff/1/ipa.c#newcode1041
>>>>>> ipa.c:1041: vnode->externally_visible = false;
>>>>>> Better to add a simple loop after varpool_node_linking to merge the
>>>>>> attribute in l-ipo.c assuming the linking happens after local visibility
>>>>>> pass.
>>>>>
>>>>> We can merge in the varpool_node_link, but we still need to change
>>>>> here as the attribute will be overwritten here in the whole_program
>>>>> pass.
>>>>>
>>>>>>
>>>>>> http://codereview.appspot.com/4798045/
>>>>>>
>>>>>
>>>>
>>>
>>
>
Xinliang David Li - July 21, 2011, 9:04 p.m.
Ok.

David

On Thu, Jul 21, 2011 at 1:45 PM, Rong Xu <xur@google.com> wrote:
> Please review the new patch attached to this email.
>
> thanks,
>
> -Rong
>
> On Thu, Jul 21, 2011 at 12:44 PM, Rong Xu <xur@google.com> wrote:
>> wait a second. this still won't work if we disable the whole-program
>> pass (like my original change, the visibility analysis change won't
>> kick in). we also need to change varpool_node_link() to merge the
>> local attribute.
>>
>> -Rong
>>
>> On Thu, Jul 21, 2011 at 12:35 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Thu, Jul 21, 2011 at 12:27 PM, Rong Xu <xur@google.com> wrote:
>>>> this is a good point. ipa_discover_readonly_nonaddressable_vars() is
>>>> called in two passes. whole-program (whole program visibility
>>>> analysis) and static-var. The one in whole-program is ok here as it is
>>>>  bundled together with the analysis. the invocation in static-var can
>>>> go wrong.
>>>>
>>>> should we add a check for COMDAT flag also? like
>>>
>>> Probably not -- only non referenced comdat vars will be marked as not needed.
>>>
>>> David
>>>
>>>> if ((vnode->needed || (L_IPO_COMP_MODE && DECL_COMDAT (node->decl)))
>>>>    && varpool_node_externally_visible_p) (
>>>>
>>>> Thanks,
>>>>
>>>> -Rong
>>>> On Thu, Jul 21, 2011 at 11:59 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> On Thu, Jul 21, 2011 at 11:32 AM, Rong Xu <xur@google.com> wrote:
>>>>>> On Thu, Jul 21, 2011 at 11:09 AM,  <davidxl@google.com> wrote:
>>>>>>>
>>>>>>> http://codereview.appspot.com/4798045/diff/1/ipa.c
>>>>>>> File ipa.c (right):
>>>>>>>
>>>>>>> http://codereview.appspot.com/4798045/diff/1/ipa.c#newcode1034
>>>>>>> ipa.c:1034: {
>>>>>>> Has varpool node linking happened at this point? If not, the new code
>>>>>>> here is not excersised.
>>>>>>
>>>>>> This functions is called in multiple places: local pass and
>>>>>> whole_program pass. varpool_node_link is b/w these two passes. the
>>>>>> varpool node attribute is set before the use in
>>>>>> ipa_discover_readonly_nonaddressable_vars()
>>>>>
>>>>> So the code relies on the second visibility pass to get things right
>>>>> -- if that pass is disabled things can go wrong (if the default
>>>>> visibility value when vnode is created is true, LIPO mode will get
>>>>> pessemitic result; if the default is false, LIPO mode will still get
>>>>> wrong result if only local visiblity pass is run).
>>>>>
>>>>> A better fix might be simply do not check 'needed' bit for comdat
>>>>> varnode in LIPO mode and always run the visibiity checker:
>>>>>
>>>>> if ((vnode->needed || L_IPO_COMP_MODE)
>>>>>   && varpool_node_externally_visible_p (...
>>>>>  )
>>>>>
>>>>>
>>>>> David
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> http://codereview.appspot.com/4798045/diff/1/ipa.c#newcode1041
>>>>>>> ipa.c:1041: vnode->externally_visible = false;
>>>>>>> Better to add a simple loop after varpool_node_linking to merge the
>>>>>>> attribute in l-ipo.c assuming the linking happens after local visibility
>>>>>>> pass.
>>>>>>
>>>>>> We can merge in the varpool_node_link, but we still need to change
>>>>>> here as the attribute will be overwritten here in the whole_program
>>>>>> pass.
>>>>>>
>>>>>>>
>>>>>>> http://codereview.appspot.com/4798045/
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Patch

2011-07-21   Rong Xu  <xur@google.com>

	* ipa.c: In LIPO mode, call varpool_externally_visible_p()
        to set the externally visible attribute even for varpool
        nodes that not marked as needed.
	* l-ipo.c: Merge the externally visible attribute for varpool
        nodes..

Index: ipa.c
===================================================================
--- ipa.c	(revision 176535)
+++ ipa.c	(working copy)
@@ -1024,7 +1024,7 @@ 
     {
       if (!vnode->finalized)
         continue;
-      if (vnode->needed
+      if ((vnode->needed || L_IPO_COMP_MODE)
 	  && varpool_externally_visible_p
 	      (vnode, 
 	       pointer_set_contains (aliased_vnodes, vnode)))
Index: l-ipo.c
===================================================================
--- l-ipo.c	(revision 176535)
+++ l-ipo.c	(working copy)
@@ -2127,6 +2127,11 @@ 
                          eq_node_assembler_name, NULL);
   for (node = varpool_nodes; node; node = node->next)
     varpool_link_node (node);
+
+  /* Merge the externally visible attribute.  */
+  for (node = varpool_nodes; node; node = node->next)
+    if (node->externally_visible)
+      (real_varpool_node (node->decl))->externally_visible = true;
 }
 
 /* Get the list of assembler name ids with reference bit set.  */