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/ >>>> >>> >> >
