Re: LTO/WHOPR streaming of varpool

[Jan Hubicka <hubicka at ucw dot cz>]

> [ CC list fixed.  I have not had a redhat.com address for a while now. ]
Sorry ;)
> > 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?

I am concerned here about large arrays - i.e. with GCC we bring in insn-attrtab
and other awfully large initializers and distribute them to every ltrans unit
that is referencing them (not only to one that actually will outout it to
assembly). Since we don't track references we really distribute them to _every_
ltrans unit.  This cause a lot of extra memory use in WPA and a lot more i/o.

Currently the WPA objects are about 10 times of size of the LTO objects so we are defintly
bringing in too much of context.  I already cut them to about 20% of original size, but still
there is long way to go.

It probably does not make sense to do so for "int a=10;" so I guess we should offline
into separate section all initializers exceeding some reasonable size.
> 
> > @@ -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.

Ah yes, it is copied from cgraph code where these two are sufficiently separated ;)

> > @@ -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?

Ah, it should be - i.e. if there is no cgraph only varpool.  I already have incremental patch
for this - there are more issues i.e. in way we detect what ltrans to output is broken too.
I've removed the unused parameter in my local copy.
> 
> > @@ -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.

It is consitent with cgraph_get_node.  I guess we should rename both
or perhaps step away from lazy creation of callgraph/varpool that is really
no longer needed.  I will also handle this incrementally.
> 
> Could you add a couple of test cases for this?

I will try to think of some.  Basically this is all shows up with whopr
bootstrap or SPEC2000 GCC build.  For LTO varpool streaming is already tested
by existing testcases we have, but yes, I agree we need more testsuite
coverage.

Honza