Please review the new patch attached to this email.
thanks,
-Rong
On Thu, Jul 21, 2011 at 12:44 PM, Rong Xu <x...@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 <davi...@google.com>
> wrote:
>> On Thu, Jul 21, 2011 at 12:27 PM, Rong Xu <x...@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 <davi...@google.com>
>>> wrote:
>>>> On Thu, Jul 21, 2011 at 11:32 AM, Rong Xu <x...@google.com> wrote:
>>>>> On Thu, Jul 21, 2011 at 11:09 AM, <davi...@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/
>>>>>>
>>>>>
>>>>
>>>
>>
>
2011-07-21 Rong Xu <x...@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. */
