Message ID | CAAe5K+WrDHbQxc1diCv1x85c0LT4YCg2UGnDajA=E64jZ9pS8w@mail.gmail.com |
---|---|
State | New |
Headers | show |
On Thu, Aug 28, 2014 at 1:40 PM, Xinliang David Li <davidxl@google.com> wrote: > > Do you know why the previous check is not enough ? > > cgraph_can_remove_if_no_direct_calls_and_refs_p (struct cgraph_node *node) This will return true for the external node, but it also returns true for the non-external node. The non-external node is a COMDAT, as as the comments in that routine indicate, COMDATs can be removed even if they are externally_visible. Teresa > > David > > > On Thu, Aug 28, 2014 at 1:29 PM, Teresa Johnson <tejohnson@google.com> > wrote: >> >> This patch fixes up varpool nodes after LIPO linking as we were doing >> for cgraph node references. While, here I made some fixes to the >> cgraph fixup as well (mark address taken as appropriate) and removed >> old references. The latter exposed an issue with resolved cgraph nodes >> getting eliminated when they were only referenced from vtables and the >> LIPO linking selected an external copy as the resolved node. Addressed >> this by forcing the LIPO linking to prefer the non-external copy. >> >> Passes regression testing, internal benchmark testing in progress. Ok >> for google/4_9 if that succeeds? >> >> Teresa >> >> 2014-08-28 Teresa Johnson <tejohnson@google.com> >> >> Google ref b/17038802. >> * l-ipo.c (resolve_cgraph_node): Pick non-external node. >> (fixup_reference_list): Fixup varpool references, remove old >> references, mark cgraph nodes as address taken as needed. >> >> Index: l-ipo.c >> =================================================================== >> --- l-ipo.c (revision 213975) >> +++ l-ipo.c (working copy) >> @@ -1564,6 +1564,15 @@ resolve_cgraph_node (struct cgraph_sym **slot, str >> (*slot)->rep_decl = decl2; >> return; >> } >> + /* Similarly, pick the non-external symbol, since external >> + symbols may be eliminated by symtab_remove_unreachable_nodes >> + after ipa inlining (see process_references). */ >> + if (DECL_EXTERNAL (decl1) && !DECL_EXTERNAL (decl2)) >> + { >> + (*slot)->rep_node = node; >> + (*slot)->rep_decl = decl2; >> + return; >> + } >> >> has_prof1 = has_profile_info (decl1); >> bool is_aux1 = cgraph_is_auxiliary (decl1); >> @@ -2304,31 +2313,44 @@ fixup_reference_list (struct varpool_node *node) >> int i; >> struct ipa_ref *ref; >> struct ipa_ref_list *list = &node->ref_list; >> - vec<cgraph_node_ptr> new_refered; >> + vec<symtab_node *> new_refered; >> vec<int> new_refered_type; >> - struct cgraph_node *c; >> + struct symtab_node *sym_node; >> enum ipa_ref_use use_type = IPA_REF_LOAD; >> >> new_refered.create (10); >> new_refered_type.create (10); >> for (i = 0; ipa_ref_list_reference_iterate (list, i, ref); i++) >> { >> - if (!is_a <cgraph_node> (ref->referred)) >> - continue; >> - >> - struct cgraph_node *cnode = ipa_ref_node (ref); >> - struct cgraph_node *r_cnode >> - = cgraph_lipo_get_resolved_node (cnode->decl); >> - if (r_cnode != cnode) >> + if (is_a <cgraph_node> (ref->referred)) >> { >> + struct cgraph_node *cnode = ipa_ref_node (ref); >> + struct cgraph_node *r_cnode >> + = cgraph_lipo_get_resolved_node (cnode->decl); >> new_refered.safe_push (r_cnode); >> use_type = ref->use; >> new_refered_type.safe_push ((int) use_type); >> + gcc_assert (use_type != IPA_REF_ADDR >> + || cnode->global.inlined_to >> + || cnode->address_taken); >> + if (use_type == IPA_REF_ADDR) >> + cgraph_mark_address_taken_node (r_cnode); >> } >> + else if (is_a <varpool_node> (ref->referred)) >> + { >> + struct varpool_node *var = ipa_ref_varpool_node (ref); >> + struct varpool_node *r_var = real_varpool_node (var->decl); >> + new_refered.safe_push (r_var); >> + use_type = ref->use; >> + new_refered_type.safe_push ((int) use_type); >> + } >> + else >> + gcc_assert (false); >> } >> - for (i = 0; new_refered.iterate (i, &c); ++i) >> + ipa_remove_all_references (&node->ref_list); >> + for (i = 0; new_refered.iterate (i, &sym_node); ++i) >> { >> - ipa_record_reference (node, c, >> + ipa_record_reference (node, sym_node, >> (enum ipa_ref_use) new_refered_type[i], >> NULL); >> } >> } >> >> >> -- >> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 > >
ok. The patch is fine. On Thu, Aug 28, 2014 at 1:46 PM, Teresa Johnson <tejohnson@google.com> wrote: > On Thu, Aug 28, 2014 at 1:40 PM, Xinliang David Li <davidxl@google.com> wrote: >> >> Do you know why the previous check is not enough ? >> >> cgraph_can_remove_if_no_direct_calls_and_refs_p (struct cgraph_node *node) > > This will return true for the external node, but it also returns true > for the non-external node. The non-external node is a COMDAT, as as > the comments in that routine indicate, COMDATs can be removed even if > they are externally_visible. > > Teresa > >> >> David >> >> >> On Thu, Aug 28, 2014 at 1:29 PM, Teresa Johnson <tejohnson@google.com> >> wrote: >>> >>> This patch fixes up varpool nodes after LIPO linking as we were doing >>> for cgraph node references. While, here I made some fixes to the >>> cgraph fixup as well (mark address taken as appropriate) and removed >>> old references. The latter exposed an issue with resolved cgraph nodes >>> getting eliminated when they were only referenced from vtables and the >>> LIPO linking selected an external copy as the resolved node. Addressed >>> this by forcing the LIPO linking to prefer the non-external copy. >>> >>> Passes regression testing, internal benchmark testing in progress. Ok >>> for google/4_9 if that succeeds? >>> >>> Teresa >>> >>> 2014-08-28 Teresa Johnson <tejohnson@google.com> >>> >>> Google ref b/17038802. >>> * l-ipo.c (resolve_cgraph_node): Pick non-external node. >>> (fixup_reference_list): Fixup varpool references, remove old >>> references, mark cgraph nodes as address taken as needed. >>> >>> Index: l-ipo.c >>> =================================================================== >>> --- l-ipo.c (revision 213975) >>> +++ l-ipo.c (working copy) >>> @@ -1564,6 +1564,15 @@ resolve_cgraph_node (struct cgraph_sym **slot, str >>> (*slot)->rep_decl = decl2; >>> return; >>> } >>> + /* Similarly, pick the non-external symbol, since external >>> + symbols may be eliminated by symtab_remove_unreachable_nodes >>> + after ipa inlining (see process_references). */ >>> + if (DECL_EXTERNAL (decl1) && !DECL_EXTERNAL (decl2)) >>> + { >>> + (*slot)->rep_node = node; >>> + (*slot)->rep_decl = decl2; >>> + return; >>> + } >>> >>> has_prof1 = has_profile_info (decl1); >>> bool is_aux1 = cgraph_is_auxiliary (decl1); >>> @@ -2304,31 +2313,44 @@ fixup_reference_list (struct varpool_node *node) >>> int i; >>> struct ipa_ref *ref; >>> struct ipa_ref_list *list = &node->ref_list; >>> - vec<cgraph_node_ptr> new_refered; >>> + vec<symtab_node *> new_refered; >>> vec<int> new_refered_type; >>> - struct cgraph_node *c; >>> + struct symtab_node *sym_node; >>> enum ipa_ref_use use_type = IPA_REF_LOAD; >>> >>> new_refered.create (10); >>> new_refered_type.create (10); >>> for (i = 0; ipa_ref_list_reference_iterate (list, i, ref); i++) >>> { >>> - if (!is_a <cgraph_node> (ref->referred)) >>> - continue; >>> - >>> - struct cgraph_node *cnode = ipa_ref_node (ref); >>> - struct cgraph_node *r_cnode >>> - = cgraph_lipo_get_resolved_node (cnode->decl); >>> - if (r_cnode != cnode) >>> + if (is_a <cgraph_node> (ref->referred)) >>> { >>> + struct cgraph_node *cnode = ipa_ref_node (ref); >>> + struct cgraph_node *r_cnode >>> + = cgraph_lipo_get_resolved_node (cnode->decl); >>> new_refered.safe_push (r_cnode); >>> use_type = ref->use; >>> new_refered_type.safe_push ((int) use_type); >>> + gcc_assert (use_type != IPA_REF_ADDR >>> + || cnode->global.inlined_to >>> + || cnode->address_taken); >>> + if (use_type == IPA_REF_ADDR) >>> + cgraph_mark_address_taken_node (r_cnode); >>> } >>> + else if (is_a <varpool_node> (ref->referred)) >>> + { >>> + struct varpool_node *var = ipa_ref_varpool_node (ref); >>> + struct varpool_node *r_var = real_varpool_node (var->decl); >>> + new_refered.safe_push (r_var); >>> + use_type = ref->use; >>> + new_refered_type.safe_push ((int) use_type); >>> + } >>> + else >>> + gcc_assert (false); >>> } >>> - for (i = 0; new_refered.iterate (i, &c); ++i) >>> + ipa_remove_all_references (&node->ref_list); >>> + for (i = 0; new_refered.iterate (i, &sym_node); ++i) >>> { >>> - ipa_record_reference (node, c, >>> + ipa_record_reference (node, sym_node, >>> (enum ipa_ref_use) new_refered_type[i], >>> NULL); >>> } >>> } >>> >>> >>> -- >>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413 >> >> > > > > -- > Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
Index: l-ipo.c =================================================================== --- l-ipo.c (revision 213975) +++ l-ipo.c (working copy) @@ -1564,6 +1564,15 @@ resolve_cgraph_node (struct cgraph_sym **slot, str (*slot)->rep_decl = decl2; return; } + /* Similarly, pick the non-external symbol, since external + symbols may be eliminated by symtab_remove_unreachable_nodes + after ipa inlining (see process_references). */ + if (DECL_EXTERNAL (decl1) && !DECL_EXTERNAL (decl2)) + { + (*slot)->rep_node = node; + (*slot)->rep_decl = decl2; + return; + } has_prof1 = has_profile_info (decl1); bool is_aux1 = cgraph_is_auxiliary (decl1); @@ -2304,31 +2313,44 @@ fixup_reference_list (struct varpool_node *node) int i; struct ipa_ref *ref; struct ipa_ref_list *list = &node->ref_list; - vec<cgraph_node_ptr> new_refered; + vec<symtab_node *> new_refered; vec<int> new_refered_type; - struct cgraph_node *c; + struct symtab_node *sym_node; enum ipa_ref_use use_type = IPA_REF_LOAD; new_refered.create (10); new_refered_type.create (10); for (i = 0; ipa_ref_list_reference_iterate (list, i, ref); i++) { - if (!is_a <cgraph_node> (ref->referred)) - continue; - - struct cgraph_node *cnode = ipa_ref_node (ref); - struct cgraph_node *r_cnode - = cgraph_lipo_get_resolved_node (cnode->decl); - if (r_cnode != cnode) + if (is_a <cgraph_node> (ref->referred)) { + struct cgraph_node *cnode = ipa_ref_node (ref); + struct cgraph_node *r_cnode + = cgraph_lipo_get_resolved_node (cnode->decl); new_refered.safe_push (r_cnode); use_type = ref->use; new_refered_type.safe_push ((int) use_type); + gcc_assert (use_type != IPA_REF_ADDR + || cnode->global.inlined_to + || cnode->address_taken); + if (use_type == IPA_REF_ADDR) + cgraph_mark_address_taken_node (r_cnode); } + else if (is_a <varpool_node> (ref->referred)) + { + struct varpool_node *var = ipa_ref_varpool_node (ref); + struct varpool_node *r_var = real_varpool_node (var->decl); + new_refered.safe_push (r_var); + use_type = ref->use; + new_refered_type.safe_push ((int) use_type); + } + else + gcc_assert (false); } - for (i = 0; new_refered.iterate (i, &c); ++i) + ipa_remove_all_references (&node->ref_list); + for (i = 0; new_refered.iterate (i, &sym_node); ++i) { - ipa_record_reference (node, c, + ipa_record_reference (node, sym_node, (enum ipa_ref_use) new_refered_type[i], NULL); } }