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