Ok. David
On Thu, Jul 21, 2011 at 1:45 PM, Rong Xu <x...@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 <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/ >>>>>>> >>>>>> >>>>> >>>> >>> >> >
