Re: [PATCH 1/2] Add an no_reorder attribute for LTO

[Jan Hubicka <hubicka at ucw dot cz>]

> > Yep, -fno-toplevel-reorder also ldisables some optimizations (as unreachable
> > function removal)
> 
> Actually it seemed like in my tests it only disables unreachable
> variable removal. Might have been wrong though.
> 
> > >    /* Set when function is visible by other units.  */
> > >    unsigned externally_visible : 1;
> > > +  /* Don't reorder to other symbols having this set.  */
> > > +  unsigned no_reorder : 1;
> > 
> > Is it necessary to introduce extra bit for this? It is quite rarely chcecked
> > property, what it buys over lookup_attribute calls everywhere?
> 
> There are quite a few FOR_EACH_FUNCTION/VAR loops that check it now.
> So I thought about m*n cost, m:symbols, n*number of attributes per sym.
> But perhaps that was premature optimization.
> 
> Also one idea was to eventually set it implicitly for the flag,
> this would allow -fno-toplevel-reorder per individual file in LTO
> and also eliminate some special cases. Ok but I suppose this could
> somehow add implicit attributes.

Hmm, that looks like a good argument - adding the attribute to every single
declaration would be wasteful.  Lets go with a flag then.
> 
> > > --- a/gcc/cgraphclones.c
> > > +++ b/gcc/cgraphclones.c
> > > @@ -437,6 +437,7 @@ cgraph_node::create_clone (tree decl, gcov_type gcov_count, int freq,
> > >    new_node->definition = definition;
> > >    new_node->local = local;
> > >    new_node->externally_visible = false;
> > > +  new_node->no_reorder = no_reorder;
> > 
> > In kernel case I suppose you don't need to stick no-reorder flag on clones?
> > If we produce static clone, in what case keeping noreorderness help?
> 
> For kernel no_reorder is only ever set on variables (initcalls)
> 
> > This makes no-reorder variables unremovable by unreachable function/variable removal.
> > This is not documented so in extend.texi and moreover perhaps we do not want this,
> > as user can use no_reorder && used attribute in that case?
> 
> It was like this before, so if the flag is set implicitely it will be
> convenient to use the same semantics. But I suppose implicit setting
> can also add attribute used (although that may change some warnings)

-fno-toplevel-reorder was done this way to immitate as closely as possible
the non-unit-at-a-time mode so legacy codebases do not break.  I guess
-fno-topelvel-reorder can just imply no_reorder and used attribute.
I still think it would be better tomake no_reorder to do only what name
suggests.
> 
> My tests also relied on it (but with Mike's trick that can be fixed)
> 
> Given all this do you still want the flag to be removed?

No, I guess it can stay.
> 
> > >    else
> > >      {
> > >        output_asm_statements ();
> > >  
> > >        expand_all_functions ();
> > >        output_variables ();
> > > +      output_in_order (true);
> > 
> > I would expect output_in_order to come first and perhaps replace output_asm_statements
> > (so relative order of asm statements and output in order stuff is preserved)
> 
> good point. 
> 
> > >    int current_order = -1;
> > > +  int noreorder_pos = 0;
> > >  
> > >    FOR_EACH_VARIABLE (vnode)
> > >      gcc_assert (!vnode->aux);
> > 
> > Hmm, why this is not a simple pass over the nodes array that goes first and inserts all noreorder
> > symbols into the first partition before the actual balancing starts?
> 
> I guess I was more ambitious. interleaving is also better if we start
> setting the flag implicitly, so the partioning with
> -fno-toplevel-reorder would behave like before.

I see, you want to mix toplevel/non-toplevel across partitions.  In that case we also
need to disable logic sorting partitions by size if no_reorder BBs exists in more than
one partition.

I guess this makes sense, I will look more into your implementation.

Honza
> 
> -andi