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

Andi Kleen andi@firstfloor.org
Sun Sep 14 19:54:00 GMT 2014


> 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.

> > --- 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)

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?

> >    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.

-andi



More information about the Gcc-patches mailing list