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