Re: Improve handling of COMDAT vtables and virtual functoins

[Richard Guenther <richard dot guenther at gmail dot com>]

On Tue, Oct 26, 2010 at 3:33 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> during the summit it turned out that virtual functions, despite the fact that their
> address is taken, can actually be unshared since the equivalence comparsion on pointers
> to virtual functions is not possible.
> Similarly for the virtual function tables.
>
> This patch excends the localization we already do for COMDATs with no address taken
> to handle these cases too.
> It turns out that same vtable can be extern in some units and comdat in others. ?This leads
> to problem because the linker does not see the comdat definition of the functoin and thus
> it resolves it as undefined. ?Our resolution code is bypassed then and we miss the vtable.
> This scenario is avoided by change in lto_symtab_resolve_symbols to do our own reoslution
> when linker didn't provided prevailing definition.
>
> Bootstrapped/regtested x86_64-linux. The patch brings some Firefox code size improvement
> (not overly large one, about 0.5%). OK?

THe lto-symtab.c change doesn't build:

> ? ? ?}
> ! ? /* If linker plugin also provided us resolution info and failed, keep info
> ! ? ? ?it provided. ?*/
> ! ? if (((lto_symtab_entry_t) *slot)->resolution != LDPR_UNKNOWN)
> ! ? ? return;
> !
> ! ? if (prevailing

Missing closing paren.

Ok if it bootstraps&tests with that fixed.

Thanks,
Richard.

> ? ? ?goto found;
>
> ? ?/* Do a second round choosing one from the replaceable prevailing decls. ?*/
> Index: cgraph.h
> ===================================================================
> *** cgraph.h ? ?(revision 165978)
> --- cgraph.h ? ?(working copy)
> *************** void varpool_node_set_remove (varpool_no
> *** 695,700 ****
> --- 695,701 ----
> ?void dump_varpool_node_set (FILE *, varpool_node_set);
> ?void debug_varpool_node_set (varpool_node_set);
> ?void ipa_discover_readonly_nonaddressable_vars (void);
> + bool cgraph_comdat_node_localizable (struct cgraph_node *);
>
> ?/* In predict.c ?*/
> ?bool cgraph_maybe_hot_edge_p (struct cgraph_edge *e);
> Index: lto-streamer-out.c
> ===================================================================
> *** lto-streamer-out.c ?(revision 165978)
> --- lto-streamer-out.c ?(working copy)
> *************** produce_symtab (struct output_block *ob,
> *** 2487,2493 ****
> ? ? ? ?if (DECL_EXTERNAL (node->decl))
> ? ? ? ?continue;
> ? ? ? ?if (DECL_COMDAT (node->decl)
> ! ? ? ? ? && cgraph_can_remove_if_no_direct_calls_p (node))
> ? ? ? ?continue;
> ? ? ? ?if (node->alias || node->global.inlined_to)
> ? ? ? ?continue;
> --- 2487,2493 ----
> ? ? ? ?if (DECL_EXTERNAL (node->decl))
> ? ? ? ?continue;
> ? ? ? ?if (DECL_COMDAT (node->decl)
> ! ? ? ? ? && cgraph_comdat_node_localizable (node))
> ? ? ? ?continue;
> ? ? ? ?if (node->alias || node->global.inlined_to)
> ? ? ? ?continue;
> *************** produce_symtab (struct output_block *ob,
> *** 2501,2507 ****
> ? ? ? ?if (!DECL_EXTERNAL (node->decl))
> ? ? ? ?continue;
> ? ? ? ?if (DECL_COMDAT (node->decl)
> ! ? ? ? ? && cgraph_can_remove_if_no_direct_calls_p (node))
> ? ? ? ?continue;
> ? ? ? ?if (node->alias || node->global.inlined_to)
> ? ? ? ?continue;
> --- 2501,2507 ----
> ? ? ? ?if (!DECL_EXTERNAL (node->decl))
> ? ? ? ?continue;
> ? ? ? ?if (DECL_COMDAT (node->decl)
> ! ? ? ? ? && cgraph_comdat_node_localizable (node))
> ? ? ? ?continue;
> ? ? ? ?if (node->alias || node->global.inlined_to)
> ? ? ? ?continue;
> *************** produce_symtab (struct output_block *ob,
> *** 2516,2521 ****
> --- 2516,2527 ----
> ? ? ? ?vnode = lto_varpool_encoder_deref (varpool_encoder, i);
> ? ? ? ?if (DECL_EXTERNAL (vnode->decl))
> ? ? ? ?continue;
> + ? ? ? /* COMDAT virtual tables can be unshared. ?Do not declare them
> + ? ? ? ?in the LTO symbol table to prevent linker from forcing them
> + ? ? ? ?into the output. */
> + ? ? ? if (DECL_COMDAT (vnode->decl)
> + ? ? ? ? && (!vnode->finalized || DECL_VIRTUAL_P (vnode->decl)))
> + ? ? ? continue;
> ? ? ? ?if (vnode->alias)
> ? ? ? ?continue;
> ? ? ? ?write_symbol (cache, &stream, vnode->decl, seen, false);
> *************** produce_symtab (struct output_block *ob,
> *** 2527,2532 ****
> --- 2533,2541 ----
> ? ? ? ?vnode = lto_varpool_encoder_deref (varpool_encoder, i);
> ? ? ? ?if (!DECL_EXTERNAL (vnode->decl))
> ? ? ? ?continue;
> + ? ? ? if (DECL_COMDAT (vnode->decl)
> + ? ? ? ? && (!vnode->finalized || DECL_VIRTUAL_P (vnode->decl)))
> + ? ? ? continue;
> ? ? ? ?if (vnode->alias)
> ? ? ? ?continue;
> ? ? ? ?write_symbol (cache, &stream, vnode->decl, seen, false);
> Index: ipa.c
> ===================================================================
> *** ipa.c ? ? ? (revision 165978)
> --- ipa.c ? ? ? (working copy)
> *************** ipa_discover_readonly_nonaddressable_var
> *** 598,603 ****
> --- 598,633 ----
> ? ? ?fprintf (dump_file, "\n");
> ?}
>
> + /* COMDAT functions must be shared only if they have address taken,
> + ? ?otherwise we can produce our own private implementation with
> + ? ?-fwhole-program.
> + ? ?Return true when turning COMDAT functoin static can not lead to wrong
> + ? ?code when the resulting object links with a library defining same COMDAT.
> +
> + ? ?Virtual functions do have their addresses taken from the vtables,
> + ? ?but in C++ there is no way to compare their addresses for equality. ?*/
> +
> + bool
> + cgraph_comdat_node_localizable (struct cgraph_node *node)
> + {
> + ? if ((node->address_taken && !DECL_VIRTUAL_P (node->decl))
> + ? ? ? || !node->analyzed)
> + ? ? return false;
> + ? if (node->same_comdat_group)
> + ? ? {
> + ? ? ? struct cgraph_node *next;
> +
> + ? ? ? /* If more than one function is in the same COMDAT group, it must
> + ? ? ? ? ?be shared even if just one function in the comdat group has
> + ? ? ? ? ?address taken. ?*/
> + ? ? ? for (next = node->same_comdat_group;
> + ? ? ? ? ?next != node; next = next->same_comdat_group)
> + ? ? ? if (next->address_taken && !DECL_VIRTUAL_P (next->decl))
> + ? ? ? ? return false;
> + ? ? }
> + ? return true;
> + }
> +
> ?/* Return true when function NODE should be considered externally visible. ?*/
>
> ?static bool
> *************** cgraph_externally_visible_p (struct cgra
> *** 623,628 ****
> --- 653,667 ----
> ? ?if (lookup_attribute ("externally_visible", DECL_ATTRIBUTES (node->decl)))
> ? ? ?return true;
>
> + ? /* When doing LTO or whole program, we can bring COMDAT functoins static.
> + ? ? ?This improves code quality and we know we will duplicate them at most twice
> + ? ? ?(in the case that we are not using plugin and link with object file
> + ? ? ? implementing same COMDAT) ?*/
> + ? if ((in_lto_p || whole_program)
> + ? ? ? && DECL_COMDAT (node->decl)
> + ? ? ? && cgraph_comdat_node_localizable (node))
> + ? ? return false;
> +
> ? ?/* See if we have linker information about symbol not being used or
> ? ? ? if we need to make guess based on the declaration.
>
> *************** cgraph_externally_visible_p (struct cgra
> *** 645,671 ****
> ? ? ?;
> ? ?else if (!whole_program)
> ? ? ?return true;
> - ? /* COMDAT functions must be shared only if they have address taken,
> - ? ? ?otherwise we can produce our own private implementation with
> - ? ? ?-fwhole-program. ?*/
> - ? else if (DECL_COMDAT (node->decl))
> - ? ? {
> - ? ? ? if (node->address_taken || !node->analyzed)
> - ? ? ? return true;
> - ? ? ? if (node->same_comdat_group)
> - ? ? ? {
> - ? ? ? ? struct cgraph_node *next;
> -
> - ? ? ? ? /* If more than one function is in the same COMDAT group, it must
> - ? ? ? ? ? ?be shared even if just one function in the comdat group has
> - ? ? ? ? ? ?address taken. ?*/
> - ? ? ? ? for (next = node->same_comdat_group;
> - ? ? ? ? ? ? ?next != node;
> - ? ? ? ? ? ? ?next = next->same_comdat_group)
> - ? ? ? ? ? if (next->address_taken || !next->analyzed)
> - ? ? ? ? ? ? return true;
> - ? ? ? }
> - ? ? }
>
> ? ?if (MAIN_NAME_P (DECL_NAME (node->decl)))
> ? ? ?return true;
> --- 684,689 ----
> *************** varpool_externally_visible_p (struct var
> *** 711,716 ****
> --- 729,743 ----
> ? ?if (!alias && vnode->resolution == LDPR_PREVAILING_DEF_IRONLY)
> ? ? ?return false;
>
> + ? /* As a special case, the COMDAT virutal tables can be unshared.
> + ? ? ?In LTO mode turn vtables into static variables. ?The variable is readonly,
> + ? ? ?so this does not enable more optimization, but referring static var
> + ? ? ?is faster for dynamic linking. ?Also this match logic hidding vtables
> + ? ? ?from LTO symbol tables. ?*/
> + ? if ((in_lto_p || flag_whole_program)
> + ? ? ? && DECL_COMDAT (vnode->decl) && DECL_VIRTUAL_P (vnode->decl))
> + ? ? return false;
> +
> ? ?/* When doing link time optimizations, hidden symbols become local. ?*/
> ? ?if (in_lto_p
> ? ? ? ?&& (DECL_VISIBILITY (vnode->decl) == VISIBILITY_HIDDEN
>