This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [google] fix global vars incorrectly marked as read-only in LIPO mode (issue4798045)
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/
>>>>>
>>>>
>>>
>>
>