Re: LTO/WHOPR streaming of varpool

[Diego Novillo <dnovillo at google dot com>]

[ CC list fixed.  I have not had a redhat.com address for a while now. ]

On 4/28/10 14:09 , Jan Hubicka wrote:

> this patch implements streaming of varpool and fixes several correctness issues
> with whopr (and makes LTO correct wrt unused variable removing too).
> Main issue with whopr was that it shipped variable declarations into multiple units
> and turned them into static vars creating large binarries (and invalid programs
> too).  This is now fixed and WHOPR produce pretty much same size of GCC binary
> as LTO.

Nice.  Thanks for doing this.

> There is room for cleanups left, especially in WPA, but I will do it incrementally.
> Also we should start streaming variable initializers in separate sections as we do
> for function bodies as well as list of references for each cgraph node/varpool
> to drive unused variable/function removal.

I don't follow.  Separating initializers would help remove unused
symbols in case values got propagated?

> @@ -407,6 +446,10 @@ struct GTY((chain_next ("%h.next"))) var
>    /* Set for aliases once they got through assemble_alias.  Also set for
>       extra name aliases in varpool_extra_name_alias.  */
>    unsigned alias : 1;
> +  /* Set when variable is used from other LTRANS partition.  */
> +  unsigned used_from_other_partition : 1;
> +  /* Set when variable is available in the other LTO partition.  */
> +  unsigned in_other_partition : 1;

The mix of LTO and LTRANS here is confusing.  ITYM LTRANS in both.

> +vsi_next (varpool_node_set_iterator *vsi)
> +{
> +  vsi->index++;
> +}
> +
> +/* Return the node pointed to by CSI.  */

s/CSI/VSI/

>  
> +/* Output the cgraph NODE to OB.  ENCODER is used to find the
> +   reference number of NODE->inlined_to.  SET is the set of nodes we
> +   are writing to the current file.  If NODE is not in SET, then NODE
> +   is a boundary of a cgraph_node_set and we pretend NODE just has a
> +   decl and no callees.  WRITTEN_DECLS is the set of FUNCTION_DECLs
> +   that have had their callgraph node written so far.  This is used to
> +   determine if NODE is a clone of a previously written node.  */

The comment is out of sync with the function arguments.

> @@ -558,6 +555,9 @@ lto_1_to_1_map (void)
>  	 cloned from.  */
>        if (node->global.inlined_to || node->clone_of)
>  	continue;
> +      /* Nodes without a body don not need partitioning.  */

s/don/do/

> @@ -918,7 +796,7 @@ strip_extension (const char *fname)
>     the same input file.  */
>  
>  static char *
> -get_filename_for_set (cgraph_node_set set)
> +get_filename_for_set (cgraph_node_set set, varpool_node_set vset ATTRIBUTE_UNUSED)

Why did you add VSET if it's not used?

> @@ -105,6 +105,22 @@ eq_varpool_node (const void *p1, const v
>    return DECL_UID (n1->decl) == DECL_UID (n2->decl);
>  }
>  
> +/* Return varpool node assigned to DECL without creating new one.  */
> +struct varpool_node *
> +varpool_get_node (tree decl)

Nit on semantics.  In general, we name '*_get' functions that will
create the thing we're looking for if it's not there.  Maybe rename to
varpool_for?  I don't think we are very consistent in this and it's
certainly not a written convention, so feel free to ignore.

> @@ -230,11 +297,7 @@ varpool_reset_queue (void)
>  bool
>  decide_is_variable_needed (struct varpool_node *node, tree decl)
>  {
> -  /* We do not track variable references at all and thus have no idea if the
> -     variable was referenced in some other partition or not.  
> -     FIXME: We really need address taken edges in callgraph and varpool to
> -     drive WPA and decide whether other partition might reference it or not.  */
> -  if (flag_ltrans)
> +  if (node->used_from_other_partition)
>      return true;
>    /* If the user told us it is used, then it must be so.  */
>    if ((node->externally_visible && !DECL_COMDAT (decl))
> @@ -288,17 +351,6 @@ varpool_finalize_decl (tree decl)
>  {
>    struct varpool_node *node = varpool_node (decl);
>  
> -  /* FIXME: We don't really stream varpool datastructure and instead rebuild it
> -     by varpool_finalize_decl.  This is not quite correct since this way we can't
> -     attach any info to varpool.  Eventually we will want to stream varpool nodes
> -     and the flags.
> -
> -     For the moment just prevent analysis of varpool nodes to happen again, so
> -     we will re-try to compute "address_taken" flag of varpool that breaks
> -     in presence of clones.  */
> -  if (in_lto_p)
> -    node->analyzed = true;
> -

Yay!


Could you add a couple of test cases for this?


Thanks.  Diego.